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

Added summary and total rows to top command. #94

Merged
merged 1 commit into from
Jul 24, 2018

Conversation

data-pup
Copy link
Member

@data-pup data-pup commented Jul 21, 2018

This PR makes a few changes to the top sub-command in order to introduce a summary of remaining items when the output is truncated, and a row containing a total of all items. If the retaining size is being used, then these columns will only show "..." in the emitted table.

Other minor changes include changing the name of the number field to max_items, for the sake of consistency with other sub-commands. This has also been changed from an Option<u32> into a plain u32 value. In order to prevent a breaking change from introducing truncation where there previously was none, this defaults to u32::MAX.

I refrained from doing so in this PR, but it seems like a good idea to decide on a consistent default for the maximum number of items to display. diff for example, defaults to 20, while others show 10. Once an --all flag is introduced, we can go about doing this, and update the test cases as well.

Other note: For now, I kept the TableRow struct internal to the text emission function, but this could eventually be refactored out into an entry struct similar to the DiffEntry and MonosEntry structs shortly, assuming the csv and json emission eventually use it as well.

(This relates to #88)

@data-pup
Copy link
Member Author

Rebased this off the new master after merging in #92, due to previous CI errors :)

@data-pup
Copy link
Member Author

CI issue popped up again, taking care of this over in #97 :)

Copy link
Member

@fitzgen fitzgen left a comment

Choose a reason for hiding this comment

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

Wow! Thanks @data-pup :)

@fitzgen
Copy link
Member

fitzgen commented Jul 24, 2018

I refrained from doing so in this PR, but it seems like a good idea to decide on a consistent default for the maximum number of items to display. diff for example, defaults to 20, while others show 10. Once an --all flag is introduced, we can go about doing this, and update the test cases as well.

+1 -- also don't think we should be afraid of breaking changes for what is 99% a CLI tool and isn't stable yet at that.

@fitzgen
Copy link
Member

fitzgen commented Jul 24, 2018

Other note: For now, I kept the TableRow struct internal to the text emission function, but this could eventually be refactored out into an entry struct similar to the DiffEntry and MonosEntry structs shortly, assuming the csv and json emission eventually use it as well.

Yeah, some of the earlier Emit implementations do a lot more duplicated work in each emit_* method than they should.

@fitzgen fitzgen merged commit a74b58a into rustwasm:master Jul 24, 2018
@data-pup data-pup deleted the add-top-summary branch February 28, 2019 19:49
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