-
-
Notifications
You must be signed in to change notification settings - Fork 903
Add further recommendations to the style guide #1377
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
Merged
Merged
Changes from all commits
Commits
Show all changes
4 commits
Select commit
Hold shift + click to select a range
File filter
Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
There are no files selected for viewing
This file contains hidden or 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
Oops, something went wrong.
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.
I'd be interested to see background on this, if possible. I've personally never felt deterred by a byline, and it is a nice way to credit someone who's written a major guide or document. However, perhaps there is a more quantitative study that we could reference if introducing this guidance?
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 do feel uneasy about changing docs that are attributed to a specific author. It feels like I'm putting words in their mouth that they may not agree with.
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 think it depends on the type of document and the type of change. For example, updating module docs is easier than updating a howto, since in the howto the original author might have had a more specific vision and goals.
On the other hand, having a byline is also useful to get in touch with the original author and ask for advice and reviews before making major changes (this could technically be replaced by
git blame
andCODEOWNERS
).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.
How do we credit someone who has written a major code changes? I don't think we add their name to the top of module name, so why would this be any different? This is an open source documentation not a blog.
As you said, this can be done by
git blame
andcodeowners
. However I would like for us to rethink the notion that the original author is the only one capable of reviewing/giving advice. Their advice may be sought, but should not be the only advice that matters, and should not be the only advice we rely on. Other contributors' review and advice should be taken into consideration equally.Uh oh!
There was an error while loading. Please reload this page.
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.
My experience in CPython is that personal ownership of areas has sometimes given rise to conflicts when other contributors wanted to meaningfully participate (without always bowing to the original author's preferences). So I'm in favor of this guideline.
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 agree with @pitrou.
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.
@JelleZijlstra I am supportive of this for existing documentation. However, this PR is concerning new documentation 😄
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.
@AA-Turner The byline style recommendation is related to new documentation. We have seen that bylines in certain areas of the documentation has caused conflicts where an individual acts as owner of the community resource. We see similar behavior when we look at academic publishing: https://pubmed.ncbi.nlm.nih.gov/31161378/
My general feeling is that reference documentation should not have bylines. As others have mentioned
git blame
orgit log
can offer recognition. If someone creates a new "How to", I think it would be reasonable to recognize in release notes/what's new as we do code contributions.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.
True. We could also consider removing bylines from existing documentation, or replacing them with something more indefinite like "Originally written by Jelle Zijlstra; maintained by the community". However, that's out of scope for this change.
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 like the suggestion @JelleZijlstra for future wording: