Skip to content

Comments

Show contribution#4

Closed
uliska wants to merge 6 commits intotimvink:masterfrom
uliska:show-contribution
Closed

Show contribution#4
uliska wants to merge 6 commits intotimvink:masterfrom
uliska:show-contribution

Conversation

@uliska
Copy link
Contributor

@uliska uliska commented Feb 25, 2020

Merge authors and show contribution

Sometimes authors commit with different email addresses or
change them over time. This commit changes the key in the authors
dictionary to be commit.author.name to merge such "virtual committers"
(if they have differing user names there's not much we can do about it).

(This is again on top of #2 and #3, although it is conceptually independent from them)

(I don't intend to be nitpicking about whitespace,
but without a separate commit the following commits
would become dirty).
The "authors" return value should be a list (of author dicts),
but in case of failure it returned an empty string.

This was incorrectly handled in Util.summarize(), but
fortunately with no negative effect:
- iterating over a string returns its characters, but
- iterating over an empty string behaves like
  iterating over an empty dict.
Create a cache dictionary, with file paths as key.
Util.get_authors() is called twice, and there's no need
to call Git twice if we can cache the results.
This has been inadvertently been overwritten by copy/paste
Closes timvink#1

This flag for the git.Repo initializer will search parent directories
for a valid Git repository too.
Sometimes authors commit with different email addresses or
change them over time. This commit changes the key in the authors
dictionary to be commit.author.name to merge such "virtual committers"
(if they have differing user names there's not much we can do about it).

In addition this commit also displays the contribution percentage
(since that has already been calculated), but only if there's more than
one contributor to the page.
Eventually I think this should be made configurable (switch on/off
and a template), but this is a more involved consideration about
the interface that I don't want to "do" without a discussion.
@timvink
Copy link
Owner

timvink commented Feb 26, 2020

Hi @uliska, thanks again! This MR is actually two things.

  1. I disagree with using the name instead of email and the unique key. I would argue thatemail is more unique, as in some environments users use their full name, and sometimes they use an alias. email also has the added benefit of preventing duplicates. Also, this change break behaviour for current users (it might change the display). If this is a feature you would like, we could create an option like aggregate_authors_on with choices email and name (defaulting to current email. Let's discuss in a separate issue, so we can also make a separate PR for it.

  2. Adding contribution percentage is cool, but should be behind an option, because it will break current user experience. If you want to see how to add an option see code of https://github.com/timvink/mkdocs-git-revision-date-localized-plugin. Also, there should be a unit test for this as well.

In short, I recommend removing 1) for now, making it a separate PR. And then either closing or improving on 2)

@uliska
Copy link
Contributor Author

uliska commented Feb 26, 2020 via email

@uliska
Copy link
Contributor Author

uliska commented Feb 26, 2020

I'll close this PR now and will open two new ones when I'm ready.

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