-
-
Notifications
You must be signed in to change notification settings - Fork 3.3k
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
Only show authors' email addresses if so configured in git-authors #6494
Merged
Merged
Changes from all commits
Commits
File filter
Filter by extension
Conversations
Failed to load comments.
Jump to
Jump to file
Failed to load files.
Diff view
Diff view
There are no files selected for viewing
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
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.
One more spot: We don't need whitespace control here. Please remove the
-
on all tags that you added.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.
Does that include the one instance of
{{- author.name -}}
that I duplicated? In other words, should the hyphens be dropped from that one as well?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. The following line is the only line where this is necessary, because we want the
,
to stick to the author name, but this is a line you did not touch, which is why I meant to remove them from all lines you touched.mkdocs-material/src/templates/partials/source-file.html
Line 60 in 2ca5cf0
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.
Also, I am not so sure about that. If I drop all instances of
-
that I introduced here, then ifshow_email_address
istrue
, I end up withAnd I'm pretty sure we don't want that comma on its own line, because it gets rendered as
... and the thus-appearing extra whitespace before the comma is extraneous.
As the patch is currently proposed, what we end up with is
... which is what we want, I think. Would you agree?
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.
I've just pushed 7f0cdd2 which is the most minimal patch that I can think of, with no whitespace management added at all. But that introduces the extra space before the comma, no matter if we're rendering a link or not. So please do not merge that as-is.
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.
That fixes things only for
show_email_address: false
; but still breaks (i.e. introduces the extraneous whitespace) for the default ofshow_email_address: true
.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.
This does the right thing, regardless of how
show_email_address
is set:Okay if I update my topic branch with that?
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.
I'm sure you don't need all of those whitespace control characters, but okay, let's keep the whitespace control. I'll look into in the future. We should keep whitespace control to a minimum, and only use it where absolutely necessary.
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.
I sympathise with that, but if you instruct me to "remove the
-
on all tags that [I] added" and simultaneously "we want the,
to stick to the author name", I can only do one xor the other. :)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.
Haha yeah, sorry for that. Entirely my fault. I'm sorry for the back and forth. Is it okay to merge now?