-
Notifications
You must be signed in to change notification settings - Fork 37.7k
Docs: Update CONTRIBUTING.md #9542
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
Conversation
the pull request affects. Examples: | ||
the pull request affects. Valid areas as: | ||
|
||
- *Consensus* for changes to consensus critical code |
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.
We also have repository tags for this level of detail. Also the habit so far has been to enclose things in square brackets.
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.
yep - labels can be used by maintainers to correctly categorize the issues/PRs (and contain many other categories such as 'Questions and Help', 'Upstream', etc). This advice is for individual contributors to mark their own PRs with the component or area they're touching.
Looking at the open PRs, there doesn't seem to be overwhelming consensus one way or the other for square brackets, so I've left the examples below as they were.
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's not entirely correct, where they are prefixed, it is usually with brackets, it's just that mostly people dont bother adding prefixes.
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 tend to always use <component>: <title>
as in commit messages of the Linux kernel and many related projects.
This is not really critical, but as jtimon points out there is a lot of confusion over "Trivial" tags for PR's. I think that we'd save ourselves a lot of grief if we adopted a policy that was more in line with how people typically (and in many other projects) use "trivial" tags. Changing the contributing.md isn't going to change what people making their first PR do, as they may not have even read that. Usually, a trivial tag refers to a PR that either changes only document to fix typos or is a minor code change that is easily reviewable and has no visible behavior change. For example, I would tag the code int a = 1+1; to int a = 2; as a Trivial change but not something like atomic_int a = 1;
++a; to atomic_int a = 0;
++a;
++a; as there is a small behavior change if someone else (from another thread observing a) would see a difference. Other sorts of things I would tag as Trivial are namespace visibility, de-duplication, etc. I think the other benefit of using "Trivial" to tag something that you think is trivial is that it is an assertion that the PR does not have any major affect, and if someone does see that it does have an effect then the PR should not be accepted (or at least re-thought). For things that are "Trivial" in the sense @jnewbery is proposing, perhaps we can use either "DocOnly", "Typo", or "NonCode" as tags. edit: I think ultimately it comes down to however @laanwj wants to manage it, but it would be nice to have a tag that means "Trivial" in the sense I am suggestion. ("Minor"? "Obvious"?) |
@JeremyRubin - I don't feel strongly one way or the other. I just added this text because I've seen a few PRs where maintainers have said that trivial should just be used for non-code changes, and that didn't seem to be documented anywhere. |
- *Qt* for changes to bitcoin-qt | ||
- *Mining* for changes to the mining code | ||
- *Net* or *P2P* for changes to the peer-to-peer network code | ||
- *Refactor* for code refactors |
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 still prefixed with a module. E.g. [qt] Wallet refactor
The prefix is imo just an indication which folder or file you are touching in that pull.
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.
ok, removed Refactor
@JeremyRubin tend to agree mostly there
Mostly, anything that doesn't change generated executable could be regarded as trivial. Not trivial:
|
Updated definition of trivial to reflect #9542 (comment) |
Looks good to me now. |
Update CONTRIBUTING.md to document the different components. Notably, trivial should only be used for PRs that do not change the code.
9b4f147
to
c70622e
Compare
commits squashed |
c70622e Docs: Update CONTRIBUTING.md (John Newbery)
@JeremyRubin explained my perspective very well. |
c70622e Docs: Update CONTRIBUTING.md (John Newbery)
c70622e Docs: Update CONTRIBUTING.md (John Newbery)
c70622e Docs: Update CONTRIBUTING.md (John Newbery)
Update CONTRIBUTING.md to document the different components.
Notably, trivial should only be used for PRs that do not change the code.