-
Notifications
You must be signed in to change notification settings - Fork 1.2k
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
Filter authors by commit count #1059
base: main
Are you sure you want to change the base?
Conversation
bin/git-summary
Outdated
echo " authors : $(number_of_authors)" | ||
format_authors <${authors_file} | ||
format_authors |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Since AUTHOR_LISTING and format_authors are in the same function, could we pass AUTHOR_LISTING as an argument to format_authors?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Yes, now both format_authors
and number_of_authors
take the listing as an argument.
{ args[NR] = $0; sum += $0 } | ||
END { | ||
for (i = 1; i <= NR; ++i) { | ||
if (strtonum(args[i]) < commitLimit) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
The strtonum
seems to be GNU awk extension, can we find an alternative?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Seems like it's unnecessary to call strtonum
at all, the comparison should work anyway.
@@ -22,6 +25,10 @@ for arg in "$@"; do | |||
OUTPUT_STYLE="$2" | |||
shift | |||
;; | |||
--author-commit-limit) | |||
AUTHOR_COMMIT_LIMIT="$2" |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Do we need to check if the commit limit exists?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Perhaps we should. I Just copied the code for --output-style
.
The completion files should be updated as well. |
I'm thinking of scrapping this PR and add an option to limit the number of shown authors instead, perhaps with a sensible default value. That seems easier for the user. Any thoughts? |
I think either possibilities are good. I can see cases where either one makes sense. If an option is added that limits the number of shown authors, it should default to the value that preserves the output. I believe that means the default would be no limit and all contributors are listed. |
In git summary
This gives better UX when running it on very large repos.