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 option to suppress size from being printed #47

Merged
merged 3 commits into from Mar 7, 2023
Merged

Add option to suppress size from being printed #47

merged 3 commits into from Mar 7, 2023

Conversation

sanders41
Copy link
Contributor

Closes #8

Copy link
Owner

@solidiquis solidiquis left a comment

Choose a reason for hiding this comment

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

Minor change requested. Thanks for building this out! Will include in next minor release over the weekend.


/// Don't show size; disabled by default
#[arg(long)]
pub suppress_size: bool,
Copy link
Owner

Choose a reason for hiding this comment

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

Could you reword to "Omit disk usage from output; disabled by default"?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

No problem, updated in both locations.

README.md Outdated
@@ -62,6 +62,7 @@ Options:
-l, --level <NUM> Maximum depth to display
-n, --scale <NUM> Total number of digits after the decimal to display for disk usage [default: 2]
-s, --sort <SORT> Sort-order to display directory content [default: none] [possible values: name, size, size-rev, none]
--suppress-size Don't show size
Copy link
Owner

Choose a reason for hiding this comment

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

Related to other comment

@sanders41
Copy link
Contributor Author

Anything special I need to do to get the same test locally as in CI? When I run the tests locally they all pass.

Copy link
Owner

@solidiquis solidiquis left a comment

Choose a reason for hiding this comment

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

Just need a sort flag on the test to get tests to pass

#[test]
fn suppress_size() {
assert_eq!(
utils::run_cmd(&["--suppress-size", "tests/data"]),
Copy link
Owner

Choose a reason for hiding this comment

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

So the test is failing because the output isn't deterministic unless you provide the --sort option. I recommend adding --sort name to get deterministic output so that tests pass :]

Copy link
Owner

@solidiquis solidiquis left a comment

Choose a reason for hiding this comment

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

Thanks @sanders41 for the contribution! Will include in next minor version bump this weekend :]

@solidiquis solidiquis merged commit f542b33 into solidiquis:master Mar 7, 2023
@sanders41 sanders41 deleted the suppress-size branch March 7, 2023 20:22
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.

[feature] could one switch be added to suspress the size?
2 participants