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

Clarify contribution guidelines to include better descriptions for PR titles #19211

Merged
merged 1 commit into from
Dec 6, 2021

Conversation

ringabout
Copy link
Member

@ringabout ringabout commented Dec 3, 2021

In addition to an issue number, the title of a PR should contain a brief description; then users can follow the progress more easily from the title.

Thank @haxscramper

The commit messages are not very clear and it is hard to know the progress of Nim language at the moment at first sight.

ed0a4e731 fixes #16585 (#17074)
8fd1ed6df fixes #17060 (#17083)
4395a2676 fixes #17085 [backport:1.2] (#17101)
4e619a6be A few rst doc fixes (#17151)
b48a32053 fixes yet another SSL problem on Windows [backport:1.2] (#17167)
e9a287fe1 fixes #17170 (#17171)
bb0c19f42 fixes #17173 (#17213)
ab780f66e fixes #17198, DFA failure on large case stmts (#17210)
2f213db7e fixes #11225; generic sandwich problems; [backport:1.2] (#17255)
dbde97f64 fixes #16076 (#17486)
355985ac8 custom integer literals bugfixes (#17499)
c6dc9c025 fixes #17437 - crash where error reporting > 1 (#17547)
89e0e0f27 Small doc fixes in net (#17566)
a31e60155 misc fixes: build_all.sh, changelog (#17601)
444658fe5 fixes #17656 (#17657)
bd7807de4 fixes #17647 (#17667)
8aa5991be Genode platform fixes (#17521)
f8dce493d rst indentation fixes (ref #17340) (#17715)
c7b77829f IC: fixes a long standing bug about DCE set computations (#17763)
7c64e49d4 getCustomPragmaVal priority/override fixes (#17725)
8f79bc5f3 add RST highlighting for command line / shells (also fixes #16858) (#17789)
82996aee3 misc fixes: remove `forceConst` (obsolete by static), add more runnableExamples to system (#17896)
51f3ef6cb fixes #15848 [backport:1.2] (#17959)
d84a3b10b fixes #17675 (#17981)
53935b8b2 ARC: fixes memory leaks with newSeq used in a loop [backport:1.4] (#18040)
558644725 fixes #17943 (#18045)
c792154b0 Minor doc fixes; follow up to 17258 and 17259 (#18123)
f10eef29b fixes #18059 (#18140)
06232b7f2 fixes #18058 (#18162)
28f2abe1a fixes #18112 (#18165)
21f3b8539 fixes #18088 (#18209)
0a4858dc5 fixes #18220 (#18227)
f65f760de fixes #15884 (#18230)
488acd9d0 fixes #18235 - proc annotation type macro sym leak (#18249)
c51680e70 fixes #17696 (#18276)
2deb7011f fixes #17768 [backport:1.4] (#18317)
d8488e41e readAsText supports both Blob and File (fixes #18187) (#18189)
0f91b67f5 fixes #18326 (#18335)
16038d44f fixes #18320 (#18343)
6be8a6683 couple tiny typo fixes (#18344)
0d194cdbf fixes #18287 (#18346)
ceb9e3efc fixes #18240 (#18354)
0be17f5d9 fixes #18319 (#18375)
97fc95012 fixes #16270 (#18388)
19263f277 fixes #18400 (#18402)
41c29cb3a fixes #18130 (#18407)
3ceaf5c13 fixes #18030 (#18415)
1bed77731 fixes #18411 (#18432) [backport:1.4]
12da32a89 fixes #17893 (#18485)
f8519657c fixes #18469 (#18544)
01fc9e58c fixes #18550 (#18553)
58e27ebd4 fixes #12815 (#18554)
76f74fae8 std/random: fix overflow bugs; fixes #16360; fixes #16296; fixes #17670 (#18456)
faabcfa64 fixes #18558 (#18563)
2cbfc1e51 fixes #18385 (#18571)
158d7c7a7 fixes #18558 again (#18586)
fa0209609 fixes #18565 (#18593)
c86f9590f fixes #18570 (#18599)
6dc34757b fixes #18579 (#18600)
4920b0697 fixes #18543 (#18601)
562dde624 fixes #18371 (#18617)
bc14b7735 fixes #18665 DFA generator bug (#18676)
018465a23 fixes #18643 [backport:1.0] (#18678)
c70e4040b fixes #14511 [backport:1.4] (#18732)
a7cae2bda fixes #16325 [backport:1.4] (#18784)
f46569baf fixes #18494 (#18783)
0887dcc39 fixes #18786 (#18788)
06ff0e962 fixes #18769 (#18790)
e8dad482a fixes #16246 (#18800)
5c85e480a unicode operator bugfixes (#18802)
73841ae19 fixes #14165, fixes #18739, fix the second example of #6269 (#18812)
34a53e804 fixes #12642 (#18811)
3241df2a1 fixes #18858 [backport] (#18868)
c56ba3f06 fixes #18847 [backport] (#18870)
14ced06bb fixes #18863 [backport] (#18871)
0ad601d3c fixes #18856 [backport] (#18879)
e3b19cbe5 fixes #18878 (#18883)
576fece90 fixes 'lent T' inside object constructor [backport] (#18911)
c38ab3e25 fixes #18921 [backport] (#18930)
f1f1e85ec fixes #18954 (#18955)
f03872d99 rst: minor fixes (#18960)
8eef55715 fixes a 'mixin' statement handling regression [backport:1.2] (#18968)
6f15af41a fixes a regression caused by overloadable enums even though they're opt-in (#18970)
7b8a4a31a Initial CI bringup and various test fixes (#14)
181791e95 fixes regression in Nim - https://github.com/nim-lang/Nim/issues/19078

In addition to an issue number, the title of a PR should contain a brief description; then users can follow the progress more easily from the title.
@ringabout ringabout requested a review from dom96 December 3, 2021 13:43
@Vindaar
Copy link
Contributor

Vindaar commented Dec 3, 2021

And it should ideally include a few words as to why the fix actually fixes the problem.

@ringabout
Copy link
Member Author

ringabout commented Dec 4, 2021

Suggestion: to make it easier to go through notifications, include more detail about the issue in the PR title.

Ref #17547 (comment)

Suggestion: more details in commit/PR titles :)

Ref #17718 (comment)

@Araq
Copy link
Member

Araq commented Dec 4, 2021

I have no intention to follow this guideline so I cannot accept it. The problem is real, but the solution is to write some simple tool that makes "git log" more useful.

@disruptek
Copy link
Contributor

Wouldn't it be easier for you to write a tool to compose commit messages that any git user can, uh, use?

@ringabout
Copy link
Member Author

ringabout commented Dec 4, 2021

I have no intention to follow this guideline so I cannot accept it. The problem is real, but the solution is to write some simple tool that makes "git log" more useful.

Ok. Then I would like this PR to be open for a while, so people can still discuss a better solution.

but the solution is to write some simple tool that makes "git log" more useful.

Well it is also notification-friendy and review-friendly for people watching this repo as @dom96 said before. #17547 (comment)

@ringabout
Copy link
Member Author

ringabout commented Dec 4, 2021

How about I change should to may or recommend containing a description in the title (not mandate)?

@Vindaar
Copy link
Contributor

Vindaar commented Dec 4, 2021

I have no intention to follow this guideline so I cannot accept it. The problem is real, but the solution is to write some simple tool that makes "git log" more useful.

How is git log supposed to help with the case where there simply is no information present in git about what is being fixed and more importantly no information is present about how and why something is a fix? git log works plenty fine for me, especially via magit. But it doesn't help with the underlying issue of not being able to understand the fixes unless I could have written the fix myself.

Yes, a custom tool could scrape the information from the related Github issue, but imo we shouldn't rely on Github for this kind of information in the first place.

@dom96
Copy link
Contributor

dom96 commented Dec 4, 2021

Definitely in favour of this guideline. Certainly tooling would be a more reliable way to solve this though. I think there are many options we could consider:

  • A post-commit hook to parse the commit message and edit it to include more info based on the referenced issue (on Araq and contributors to have this installed, so meh)
  • A bot that rewrites PRs commit messages to include more info based on the referenced issue (could work, but would have to be done at merge-time, can bors do this? if not I guess making a plugin for it specifically would be fairly trivial)
  • A better git log that can lookup the issue references automatically (what Araq is describing I guess)

So yeah, I do sympathise with Araq's stance. Automating away this manual work as much as possible is worthwhile. But at the same time I think we can also just merge this and hope it has some impact on contributors.

Btw the title of this PR should reflect it better too IMO, I was really confused when I first read it :). Something like "Clarify contribution guidelines to include better descriptions" would be better.

@ringabout
Copy link
Member Author

Something like "Clarify contribution guidelines to include better descriptions" would be better.

Make sense.

@ringabout ringabout changed the title the title of PR should contain a brief description Clarify contribution guidelines to include better descriptions for PR titles Dec 5, 2021
@ringabout
Copy link
Member Author

ringabout commented Dec 5, 2021

But at the same time I think we can also just merge this and hope it has some impact on contributors.

IMHO, ideally we can use a PR template to suggest newcomers what to do: https://docs.github.com/en/communities/using-templates-to-encourage-useful-issues-and-pull-requests/creating-a-pull-request-template-for-your-repository
Then contributors are more likely to read our contributing guide.

See also https://github.com/crystal-lang/crystal/blob/master/.github/PULL_REQUEST_TEMPLATE/pull_request_template.md

@Araq
Copy link
Member

Araq commented Dec 5, 2021

Yes, a custom tool could scrape the information from the related Github issue, but imo we shouldn't rely on Github for this kind of information in the first place.

If you don't automate it, it'll be unreliable, so not "relying on Github" is a very bad idea.

@arnetheduck
Copy link
Contributor

Generally a tool can't solve this problem - the whole idea with a commit message is that it describes the change, such that when the next developer looks at it, the rationale for making the change in a particular are clear - a bug report rarely includes information like this - a commit line "code snipped xyz crashes the compiler" is not much more helpful than "fixes ".

A commit message is a communications tool where you're telling a story to future developers and users - they don't have the luxury of asking questions, so you have to anticipate what their questions will be - often, these questions are along the lines of "why was this design chosen" or "what flow was intended in this fix". An example is "xyz crashes compiler" (the bug report) and "generic types must be made concrete before operation X is performed" (the commit) - the latter tells the next developer how to fit the next bugfix or feature into the flow.

@Araq Araq merged commit faacd63 into devel Dec 6, 2021
@Araq Araq deleted the xflywind-patch-2 branch December 6, 2021 09:04
@Araq
Copy link
Member

Araq commented Dec 6, 2021

@narimiran pointed out that it would simplify his backporting efforts too, so I'll try to follow this new guideline.

PMunch pushed a commit to PMunch/Nim that referenced this pull request Mar 28, 2022
In addition to an issue number, the title of a PR should contain a brief description; then users can follow the progress more easily from the title.
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.

6 participants