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

add raw level arg #372

Merged
merged 9 commits into from
Mar 19, 2023
Merged

add raw level arg #372

merged 9 commits into from
Mar 19, 2023

Conversation

xgdgsc
Copy link
Contributor

@xgdgsc xgdgsc commented Mar 14, 2023

partly fix #52 .

I don' t find the "translate" necessary as users should know what they are actually using if they are trying to use a compression level.

src/cli/args.rs Outdated Show resolved Hide resolved
@marcospb19
Copy link
Member

Thanks for your contribution! We really do appreciate it, here's a small change request:

I see you're simulating Option<u32> behavior by using u32::MAX as a marker for None, instead, you can provide Option<u32>, clap will understand that it is optional and now we'll be able to see in the code that this flag is indeed optional, and maybe wasn't provided.

As Figsoda said, you can also take advantage of this and use Option's methods for default values, instead of doing if-else.

I also think it should be called --level.

src/cli/args.rs Outdated Show resolved Hide resolved
src/cli/args.rs Outdated Show resolved Hide resolved
src/commands/compress.rs Outdated Show resolved Hide resolved
src/commands/compress.rs Outdated Show resolved Hide resolved
src/commands/compress.rs Outdated Show resolved Hide resolved
src/commands/compress.rs Outdated Show resolved Hide resolved
src/commands/compress.rs Outdated Show resolved Hide resolved
src/commands/compress.rs Outdated Show resolved Hide resolved
src/commands/compress.rs Show resolved Hide resolved
src/commands/compress.rs Outdated Show resolved Hide resolved
src/commands/compress.rs Outdated Show resolved Hide resolved
tests/integration.rs Outdated Show resolved Hide resolved
tests/integration.rs Outdated Show resolved Hide resolved
src/commands/compress.rs Outdated Show resolved Hide resolved
src/commands/compress.rs Outdated Show resolved Hide resolved
Copy link
Member

@marcospb19 marcospb19 left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I reviewed the compression levels for each format and three of them need adjustment:

src/commands/compress.rs Show resolved Hide resolved
src/cli/args.rs Outdated Show resolved Hide resolved
src/commands/compress.rs Outdated Show resolved Hide resolved
src/commands/compress.rs Outdated Show resolved Hide resolved
@figsoda
Copy link
Member

figsoda commented Mar 18, 2023

you might also want to allow clippy::too-many-arguments just to make clippy happy

Copy link
Member

@marcospb19 marcospb19 left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Looks great, thanks!

@marcospb19 marcospb19 merged commit e0391a8 into ouch-org:main Mar 19, 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

Successfully merging this pull request may close these issues.

Add compression level configuration flags
3 participants