Skip to content

Conversation

jnewbery
Copy link
Contributor

Update CONTRIBUTING.md to document the different components.

Notably, trivial should only be used for PRs that do not change the code.

the pull request affects. Examples:
the pull request affects. Valid areas as:

- *Consensus* for changes to consensus critical code
Copy link
Contributor

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.

Copy link
Contributor Author

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.

Copy link
Contributor

@btcdrak btcdrak Jan 14, 2017

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.

Copy link
Member

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.

@JeremyRubin
Copy link
Contributor

JeremyRubin commented Jan 13, 2017

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"?)

@jnewbery
Copy link
Contributor Author

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

@fanquake fanquake added the Docs label Jan 13, 2017
- *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
Copy link
Member

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.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

ok, removed Refactor

@laanwj
Copy link
Member

laanwj commented Jan 16, 2017

@JeremyRubin tend to agree mostly there
These changes to the code are clearly trivial:

  • Comment only
  • Whitespace only
  • Changes only variable names
  • Typos in logging and messages

Mostly, anything that doesn't change generated executable could be regarded as trivial.

Not trivial:

  • Refactors (change of function arguments, moves)
  • Changes in behavior

@jnewbery
Copy link
Contributor Author

Updated definition of trivial to reflect #9542 (comment)

@laanwj
Copy link
Member

laanwj commented Jan 18, 2017

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.
@jnewbery jnewbery force-pushed the CONTRIBUTINGcomponents branch from 9b4f147 to c70622e Compare January 18, 2017 19:05
@jnewbery
Copy link
Contributor Author

commits squashed

@laanwj laanwj merged commit c70622e into bitcoin:master Jan 19, 2017
laanwj added a commit that referenced this pull request Jan 19, 2017
c70622e Docs: Update CONTRIBUTING.md (John Newbery)
@jnewbery jnewbery deleted the CONTRIBUTINGcomponents branch January 19, 2017 14:42
@jtimon
Copy link
Contributor

jtimon commented Jan 24, 2017

@JeremyRubin explained my perspective very well.
I use the trivial tag to make code changes that should be trivial to review and don't change functionality, maybe even for things like #9176. If it takes more than a few minutes to review, I think "NACK this is not trivial" or "Nit remove the trivial label, this is not trivial".
Anyway, should have left this comments some time ago and what has been merged is more formal.

codablock pushed a commit to codablock/dash that referenced this pull request Jan 21, 2018
c70622e Docs: Update CONTRIBUTING.md (John Newbery)
andvgal pushed a commit to energicryptocurrency/gen2-energi that referenced this pull request Jan 6, 2019
c70622e Docs: Update CONTRIBUTING.md (John Newbery)
CryptoCentric pushed a commit to absolute-community/absolute that referenced this pull request Feb 27, 2019
c70622e Docs: Update CONTRIBUTING.md (John Newbery)
@bitcoin bitcoin locked as resolved and limited conversation to collaborators Sep 8, 2021
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
Projects
None yet
Development

Successfully merging this pull request may close these issues.

7 participants