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

Review Pls #1

Closed
bash opened this issue Apr 7, 2023 · 5 comments
Closed

Review Pls #1

bash opened this issue Apr 7, 2023 · 5 comments
Assignees

Comments

@bash
Copy link
Owner

bash commented Apr 7, 2023

No description provided.

@bash
Copy link
Owner Author

bash commented Apr 7, 2023

@janhohenheim

@janhohenheim
Copy link
Sponsor

janhohenheim commented Apr 18, 2023

  • Derive Deref and DerefMut for IconSizes and remove the .0 from sizes.0
  • IMO these lines violate the SRP and should be in their own function
  • Replace the PathBuf::push call with let output_path: PathBuf = [out_dir, file_name].iter().collect();, see the docs
  • Replace the .unwrap calls with .expect with a nice message
  • Rename buf to buffer
  • Move the example in the docs to an actual example
  • What happens when the input images are not squares?
  • Reduce code duplication by making add_source_file call add_source_files
  • IconSizes::new requires 'static input, limiting its usefulness in e.g. GUIs with user input. It uses a Cow internally, so this is unnecessary

@bash
Copy link
Owner Author

bash commented Apr 18, 2023

Thaaanks so much for your review ❤️. You indeed caught some things I have missed :)

I have addressed most of the comments and checked them off the list.

Derive Deref and DerefMut for IconSizes and remove the .0 from sizes.0.

I can only implement Deref as my underlying Cow might be a non-mutable reference :)

  • IconSizes::new requires 'static input, limiting its usefulness in e.g. GUIs with user input. It uses a Cow internally, so this is unnecessary

I only need the Cow so that I can create IconSizes in const contexts. In that case 'static is indeed required.

If you have non-static data, you can use this From impl:

impl<'a, I> From<I> for IconSizes
where
    I: IntoIterator<Item = &'a u32>

@janhohenheim
Copy link
Sponsor

Aaah I see, fair enough. Consider the review finished in that case :)

@bash
Copy link
Owner Author

bash commented Apr 20, 2023

Oh, I forgot to close this :) Thanks again for the review ❤

@bash bash closed this as completed Apr 20, 2023
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

No branches or pull requests

2 participants