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.

Sign up for GitHub

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

Jump to bottom

summarize (feature): adds percent-above cli argument #36

Merged
merged 5 commits into from
Apr 30, 2019
Merged

summarize (feature): adds percent-above cli argument #36

merged 5 commits into from
Apr 30, 2019

Conversation

amanjeev
Copy link
Member

Closes #32.

Please let me know if I am doing something that is naive.

I also took the leeway to add a CHANGELOG.md. Please let me know if you suggest I not add that.

bjorn3
bjorn3 suggested changes Apr 28, 2019
View reviewed changes
summarize/src/main.rs Show resolved Hide resolved
CHANGELOG.md Outdated Show resolved Hide resolved
summarize/src/main.rs Outdated Show resolved Hide resolved
wesleywiser
wesleywiser requested changes Apr 28, 2019
View reviewed changes
Copy link
Member

@wesleywiser wesleywiser left a comment

Choose a reason for hiding this comment

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

Overall this looks pretty good. I really like the Changelog idea!

summarize/src/main.rs Outdated Show resolved Hide resolved
summarize/src/main.rs Outdated Show resolved Hide resolved
summarize/src/main.rs Outdated Show resolved Hide resolved
summarize/src/main.rs Outdated Show resolved Hide resolved
CHANGELOG.md Outdated Show resolved Hide resolved
@amanjeev
Copy link
Member Author

Thank folks! Do you care if I push more commits or should I decline this PR and open a new one?

@wesleywiser
Copy link
Member

I'd say either use git commit -a --amend if you're comfortable with that, otherwise, push new commits into this branch.

wesleywiser
wesleywiser approved these changes Apr 30, 2019
View reviewed changes
Copy link
Member

@wesleywiser wesleywiser left a comment

Choose a reason for hiding this comment

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

I'm going to open an issue about finding a better error handling strategy but this looks good to me as-is. Nice work!

@wesleywiser wesleywiser merged commit e0d7945 into rust-lang:master Apr 30, 2019
@wesleywiser wesleywiser mentioned this pull request Apr 30, 2019
Open
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@Darksonn Darksonn Darksonn left review comments

@bjorn3 bjorn3 bjorn3 requested changes

@wesleywiser wesleywiser wesleywiser approved these changes

Assignees

@wesleywiser wesleywiser

Labels
None yet
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.

Filter out small items from summarize table
4 participants
@amanjeev @wesleywiser @Darksonn @bjorn3