Skip to content
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

docs/development Key points: Referencing issues in commit messages #3210

Closed
wants to merge 1 commit into from

Conversation

0cjs
Copy link
Contributor

@0cjs 0cjs commented Jan 29, 2024

This change was made to my last accepted commit, and looking back it's standard in a lot of commits, so it's clearly something developers should be doing normally.

No changelog update because this is already covered by the #3197 changelog going out in the next release.

  • [✓] ran the linter to address style issues (tox -e fix)
  • [✓] wrote descriptive pull request text
  • [n/a] ensured there are test(s) validating the fix
  • [n/a] added news fragment in docs/changelog folder
  • [✓] updated/extended the documentation

This change was made to my last accepted commit, and looking back it's
standard in a lot of commits, so it's clearly something developers should
be doing normally.

No changelog update because this is already covered by the tox-dev#3197 changelog
going out in the next release.
@0cjs 0cjs requested a review from gaborbernat as a code owner January 29, 2024 04:47
@@ -12,6 +12,8 @@ section. This section contains only highlights; it's not a substitute for readin

- Lines wrapped at less than 120 characters. Lines should be wrapped at 120 characters, not the PEP-8 standard of 79.
- Variable names should be at least two characters long.
- If a commit references an issue, this should be noted in parentheses at the end of the summary line, e.g.
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 really optional, some people tend to do this but I'm not sure I want to make it a requirement.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Ah. I added this because you were updating my commit messages to do this, so I thought it was required. (I'm also assuming that, since this is the "for experienced developers section," experienced developers will do what they consider the right thing anyway if they feel they need to violate these guidelines, explain it in the PR, and negotiate what needs to be done with you.)

So we can:

  • Just drop this change if you're ok without mentioning the issue on the summary line (though I'd reckoned you were not), in which case you can just reject the PR.
  • Mark this optional, in which case I'll update the text.
  • Leave it as it is in this change if you'd prefer people do this unless they think they've got a good reason not to, in which case you can just merge the PR.

I'm good with any of the above options.

Copy link
Member

Choose a reason for hiding this comment

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

Mark it optional.

@gaborbernat
Copy link
Member

I'll close this as seems this has stalled.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants