Skip to content

Conversation

anaisbetts
Copy link
Contributor

Finish up the methods from ReleaseEntry:

TODO:

  • Generate from file
  • Evaluate staging percentage

Copy link

@KodrAus KodrAus left a 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>> {
Copy link

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));
Copy link

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();
Copy link

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.

Copy link

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.


let mut ret = ReleaseEntry {
sha256: [0; 32],
filename_or_url: file.to_str().unwrap().to_owned(),
Copy link

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(),
Copy link

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.

ret.sha256[i] = sha[i];
}

return Ok(ret);
Copy link

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 {
Copy link

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 {
Copy link

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))
    }
}

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants