-
Notifications
You must be signed in to change notification settings - Fork 9
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
Generate from file content #3
base: master
Are you sure you want to change the base?
Conversation
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I thought I'd just leave a bunch of comments around idioms you might find interesting.
// | ||
// Create new release entries | ||
impl ReleaseEntry { | ||
pub fn from_file(path: &str) -> Result<ReleaseEntry, Box<Error>> { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This could be fn from_file<P: AsRef<std::path::Path>>(path: P)
. That way we can accept anything that can be referenced as a Path
, which includes owned and borrowed strings, paths and OS strings.
For reference, that's what the sig for File::open
looks like.
pub fn from_file(path: &str) -> Result<ReleaseEntry, Box<Error>> { | ||
let mut sha256_sum = Sha256::new(); | ||
{ | ||
let file = try!(File::open(path)); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
There's a ?
operator that can be used instead of the try!
macro.
So this line could be let file = File::open(path)?;
|
||
let p = Path::new(path); | ||
let stat = try!(fs::metadata(path)); | ||
let file = p.file_name().unwrap(); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Should be able to use ?
instead of unwrapping.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
If P: AsRef<Path>
then you could do p.as_ref().file_name()?
and you could remove line 54.
sha256: [0; 32], | ||
filename_or_url: file.to_str().unwrap().to_owned(), |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Could use ?
here instead of unwrap
.
sha256: [0; 32], | ||
filename_or_url: file.to_str().unwrap().to_owned(), | ||
length: stat.len(), | ||
version: Version::parse("0.0.0").unwrap(), |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Could use ?
here instead of unwrap
.
} | ||
|
||
return Ok(ret); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
In Rust, the last expression statement in a block is the return. If that statement doesn't end with ;
then its value is returned, otherwise ()
is.
So this could simply be Ok(ret)
.
If it was Ok(ret);
then you'd get an error.
// | ||
|
||
impl ReleaseEntry { | ||
fn sha256_to_string(&self) -> String { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I think this could be self.sha256.into_iter().map(|x| format!("{:02x}", x)).collect()
.
Because collect
is magic.
}); | ||
} | ||
|
||
fn escape_filename(&self) -> String { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Not sure how much you care about allocations here, but you might find the Cow
type interesting. It would let you express a return type here that might be borrowed (in case SCHEME.is_match
) or owned (in case it needs to be encoded).
It would basically look like:
fn escape_filename(&self) -> Cow<str> {
if match {
Cow::Borrowed(&self.filename_or_url)
} else {
Cow::Owned(utf8_percent_encode(&self.filename_or_url))
}
}
Finish up the methods from ReleaseEntry:
TODO: