Skip to content
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

Open
wants to merge 5 commits into
base: master
Choose a base branch
from
Open

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.

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.

}

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