CONTRIBUTING: Added instructions for PR workflow

[chrysle]

Summary

This closes #1104

Pull Request Check List

• Added tests for changed code.
  Our CI fails if coverage is not 100%.
• New features have been added to our Hypothesis testing strategy.
• Changes or additions to public APIs are reflected in our type stubs (files ending in .pyi).
  • ...and used in the stub test file tests/typing_example.py.
  • If they've been added to attr/__init__.pyi, they've also been re-imported in attrs/__init__.pyi.
• Updated documentation for changed code.
  • New functions/classes have to be added to docs/api.rst by hand.
  • Changes to the signature of @attr.s() have to be added by hand too.
  • Changed/added classes/methods/functions have appropriate versionadded, versionchanged, or deprecated 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.
• Documentation in .rst files is written using semantic newlines.
• Changes (and possible deprecations) have news fragments in changelog.d.
• Consider granting push permissions to the PR branch, so maintainers can fix minor issues themselves without pestering you.

[chrysle]

@webknjaz I have addressed your comments.

[webknjaz]

I'll let Hynek decide if it's good enough and merge.

[chrysle]

I'll let Hynek decide if it's good enough and merge.

CC @hynek ;-)

[hynek]

SO busy rn

[webknjaz]

Have fun upside-down 🙃

[hynek]

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?

[chrysle]

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 git fetch commands to highlight the importance of fetching the tags and keeping the local copy up to date.

[webknjaz]

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 <details> so only those needing it would expand and read?

[hynek]

Is it a good moment to mention that I've never in my life used git fetch and to this day don't understand what it's good for when we have git pull with rebase and autostash? /o\

[webknjaz]

It's useful to synchronize the remote refs without applying them with merge. git pull is the same as git fetch + git merge. You can use it when you're working on a non-shared branch. Sometimes, it's useful to fetch if you don't intend to merge but want to have remote refs cached/updated locally so you could compare or cherry-pick things.
Naturally, if you work on a single computer and work with your own repository, your changes originate in controlled environments.
But imagine you added other forks as remotes or contribute to another project. In this case, you'd want to update the local copy of those things that you don't even have write access to. You may then want to rebase on top of upstream's main branch. But git rebase does not do network I/O — you specify refs that are already fetched into your .git/ folder. If you happen to notice that a remote's main got new commits, you cannot incorporate those into your branch that you typically push to your fork, unless you first fetch from that remote.
Since I often have several remotes added, I go for git fetch --all to synchronize all of them.
Note that things like git pull only fetch from one remote, that has the tracking branch, others remain unsynchronized.

Note on git pull --rebase --autostash — I use it a lot, mostly for the default branch, but every once in a while, it's easier to just have a preview compared to what's fetched rather than dealing with half-applied stash with conflicts. By the way, using git worktree greatly reduces the need to stash stuff.

[chrysle]

@webknjaz Thanks for the explanation! It does seem like git fetch is quite useful for various development situations and should be kept.

I'm +1 on your idea with hiding the details. Maybe from 102-109 and 47-67?

[webknjaz]

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.

[webknjaz]

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.

[webknjaz]

With the comment in another thread above, it could probably be just git fetch upstream --prune-tags.

[hynek]

Do we need --prune-tags for anything?

[chrysle]

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.

[hynek]

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. :-/

[webknjaz]

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.

[webknjaz]

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.

[hynek]

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.

[chrysle]

Thanks for looking into this! I like your suggestions and will rework my PR if @hynek agrees too.

[hynek]

So I'm trying to sieve the instructions down to what is necessary, but we're almost there.

[hynek]

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.

[hynek]

Do we need --prune-tags for anything?

[hynek]

@webknjaz

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).

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. 🤔

[webknjaz]

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.

[hynek]

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 😅

[chrysle]

Yes, this was really getting unoverseeable.

pls absolutely feel free to open another PR, but this shed is a McMansion 😅

Will try to apply some leftover feedback and hope you haven't lost your good humour ;-)!