-
-
Notifications
You must be signed in to change notification settings - Fork 360
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
CONTRIBUTING: Added instructions for PR workflow #1105
Conversation
for more information, see https://pre-commit.ci
…nto contributing-pr-workflow
Co-authored-by: Sviatoslav Sydorenko <wk.cvs.github@sydorenko.org.ua>
Co-authored-by: Sviatoslav Sydorenko <wk.cvs.github@sydorenko.org.ua>
@webknjaz I have addressed your comments. |
I'll let Hynek decide if it's good enough and merge. |
CC @hynek ;-) |
Have fun upside-down 🙃 |
As I've already mentioned on the bug report, I'm not super happy to accumulate all git knowledge in my contributing guides which I already find a bit too long (at least too long for everyone to read it ;)). Does anything speak against just linking to the official docs at https://docs.github.com/en/get-started/quickstart/contributing-to-projects? Maybe with some addendums that are attrs-specific? |
No, I don't think so. As for the addendums, I'd suggest keeping the |
I agree with @chrysle on this. In particular, that GitHub docs are incomplete and don't really describe the full workflow. How about making some bits collapsed via |
Is it a good moment to mention that I've never in my life used |
It's useful to synchronize the remote refs without applying them with merge. Note on |
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 +1 on your idea with hiding the details. Maybe from 102-109 and 47-67?
I don't have a strong opinion on this. But as it's piling up, maybe it'd make sense to have a separate "Git mastery" section at the bottom. Dunno.
I usually let my mentees go through a collection of resources on Git/contributing/reviews: https://gist.github.com/webknjaz/a5a4fb374b7579de827e6bedb93a5220 / https://gist.github.com/webknjaz/cb7d7bf62c3dda4b1342d639d0e78d79 / https://gist.github.com/webknjaz/a7362787a80067af8621a85a71746ca1 / https://gist.github.com/webknjaz/2aa9c7a43b51c1385260ff87e0cbb9e3.
There's a lot of things one could learn to be efficient with Git but overwhelming the contributors with such amount of information would be unhelpful (some would even say cruel).
I've left some notes in the diff too.
.github/CONTRIBUTING.md
Outdated
``` | ||
|
||
This is important to obtain eventually missing tags, which are needed to install the development version later on. See [#1104](https://github.com/python-attrs/attrs/issues/1104) for more information. |
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 is a bit inaccurate, though. It fetches both branches and tags. But not necessarily all.
Maybe, it makes sense to add --tags --prune-tags
. Maybe, it makes sense to specify the branch name too.
git fetch upstream main --tags --prune-tags
would probably represent what we want here.
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.
With the comment in another thread above, it could probably be just git fetch upstream --prune-tags
.
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 --prune-tags
for anything?
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 will remove any local tags that do no longer exist in the remote; I guess it's useful.
I think the --prune
option needs to be passed too for this to succeed? See git fetch 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.
yeah that seems too specific for a "get up and contribute real quick 101" for a project that never had to remove a tag. I really want to be helpful, but if we make it too complicated, people won't read / understand it. It's a tedious balance act. :-/
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.
Agreed, tags are supposed to be immutable, anyway. Also, even if tags change, it probably won't upset the contributors since they shouldn't care too much when the generated version is a little off. It's not like they are going to make official dists from that env, anyway. Though, I'd like https://github.com/python-attrs/attrs/pull/1105/files#r1146115629 to be accepted because it'll set up the local config of the Git repo to always fetch the right things w/o extra args.
.github/CONTRIBUTING.md
Outdated
$ git checkout -b my_topical_branch upstream/main | ||
``` | ||
|
||
Make your changes, push them to your fork (the remote *origin*) and publish the PR! |
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 could be accompanied with the following snippet:
$ git push -u origin feature/my-topic-branch
-u
marks the branch on the origin
remote (aka fork) as corresponding to the local one so that git push
w/o args would pick it up automatically.
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 removed the branch name because it feels tedious and it's unclear to me what problem it's supposed to solve. happy to be corrected, but I'll try to keep it as simple as possible.
Thanks for looking into this! I like your suggestions and will rework my PR if @hynek agrees too. |
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.
So I'm trying to sieve the instructions down to what is necessary, but we're almost there.
.github/CONTRIBUTING.md
Outdated
$ git checkout -b my_topical_branch upstream/main | ||
``` | ||
|
||
Make your changes, push them to your fork (the remote *origin*) and publish the PR! |
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 removed the branch name because it feels tedious and it's unclear to me what problem it's supposed to solve. happy to be corrected, but I'll try to keep it as simple as possible.
.github/CONTRIBUTING.md
Outdated
``` | ||
|
||
This is important to obtain eventually missing tags, which are needed to install the development version later on. See [#1104](https://github.com/python-attrs/attrs/issues/1104) for more information. |
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 --prune-tags
for anything?
Any chance we could make these resources something more permanent, link to them, and simplify what's in here? Like, even I can't follow all instructions and it's just supposed to get people started to quickly contribute. 🤔 |
These gists contain a lot of in-depth materials that are also sometimes redundant. I use them to collect links for my mentees mostly so it's best not to rely on what's inside by linking in the contributing doc but you could extract interesting things to store separately that you could link or include. I mostly posted them because it seems like you or @chrysle might want to lurk inside at some point. |
Co-authored-by: Sviatoslav Sydorenko <wk.cvs.github@sydorenko.org.ua>
ugh i've applied a suggestion twice now which is the perfect signal we're done here. pls absolutely feel free to open another PR, but this shed is a McMansion 😅 |
Yes, this was really getting unoverseeable.
Will try to apply some leftover feedback and hope you haven't lost your good humour ;-)! |
Summary
This closes #1104
Pull Request Check List
Our CI fails if coverage is not 100%.
.pyi
).tests/typing_example.py
.attr/__init__.pyi
, they've also been re-imported inattrs/__init__.pyi
.docs/api.rst
by hand.@attr.s()
have to be added by hand too.versionadded
,versionchanged
, ordeprecated
directives.The next version is the second number in the current release + 1.
So if the current version on PyPI is 23.1.0, the next version is gonna be 23.2.0.
.rst
files is written using semantic newlines.changelog.d
.