Rework CONTRIBUTING guidelines #3549
Conversation
Give better recommendations for contributors on how to propose start the work, write commit messages, and handle review by explicitly stating expectations, however make it clear that maintainers will help you if you have issues. Write down the guidelines for maintainers on how they should handle contributions and interact with other maintainers to help maintaining good code quality level by listing maintainers responsibilities and providing suggestions on how to handle various situations.
| #### Creating commits | ||
|
|
||
| When creating a commit, follow these general rules: | ||
| - Try to limit the first line (title) of the commit message to 50 characters, |
There was a problem hiding this comment.
I think that a link here to the Git standards this is taken from would be appreciated.
There was a problem hiding this comment.
50 characters is not that hard to google, but the source is a bit vague on that. I'd add that multiple editors (nvim, helix) do that sort of indication by default. The 72 chars body is from kernel and in general a lot of inspired by kernel devs, since they know how to write commits.
https://www.kernel.org/doc/html/latest/process/submitting-patches.html#describe-your-changes
There was a problem hiding this comment.
Having this linked in this document would be nice
| who submitted the patch. Interested maintainers could help push the work over | ||
| the finish line, but teaching other maintainers should be preferred. | ||
|
|
||
| When contributor is _regular_ in winit, the maintainer should slowly start |
There was a problem hiding this comment.
This paragraph seems unnecessary and overly restrictive, so some context would be appreciated.
There was a problem hiding this comment.
I mean, it's a regular process of evolving. It's present in mostly every project I worked in.
There was a problem hiding this comment.
I guess what I'm asking is, what bad behavior is this intended to stop? I feel like this is a solution in search of a problem.
I was thinking on adding |
While this would be a good solution I would prefer it if it was stated explicitly here. |
There was a problem hiding this comment.
I think in general this looks good (except commit messages), I would probably like to proof read it but otherwise I believe this is a great improvement.
Personally I'm not in favor of documenting changes in commit messages. Documenting changes in PRs has to be done anyway, as it is required to actually get anything merged, otherwise nobody would know whats going on. So to me this is duplicating that work.
I do believe that documenting changes in commit messages is an improvement, but it comes with significant drawbacks. Most notably its a lot more work and according to the new guidelines, that work has to be taken on by maintainers for contributors. Those tradeoffs ought to be weighed and discussed.
Additionally, I would like to point out that this isn't a particularly popular way to do things anymore, e.g. apart from Linux and security related projects I've not seen it often in the Rust open source ecosystem. I believe that today, in general, documenting changes has moved from commit messages to PR's and issues, which I personally don't mind.
I also believe that squashing is fine as long as its not worth to preserve the commit history, which it often isn't in Winit, especially for smaller changes (apart from the fact that commit history is still preserved in the PR itself).
Good commit messages are good though and have minimal tradeoff, I don't see a reason to not have good commit messages. But I do think that even 60 characters is a very traditional limit that doesn't really make sense with modern screens and UIs anymore. Again, we should discuss the tradeoffs here.
I want to clarify that I do actually believe that clearly documenting changes in the commit message is always going to be an improvement. But every improvement has tradeoffs that we should weigh and discuss.
Imo, the Rust open source ecosystem in general doesn't require documenting changes in commit messages (not even the Rust project itself), probably because its a much newer ecosystem, therefor I don't see why we should be different here considering the tradeoffs.
I already do that myself for each PR I merge on the span of 3 years. I don't mind continue doing so, it usually takes like a few minutes. Keep in mind that it's recommended to write correct messages, but to not demotivate contributors, I don't mind fixing them myself.
Squashing is fine, the thing is that this option can't be used reliably by some parties, once there will be indication that we can write commit messages so things look consistent, I'd bring it back.
Well, it's the topic line, brevity is more important. If you need more than you likely need to split commit into multiples.
That sounds like their issue and the fallout of using bors. Though, people generally document why they make a particular change. The thing is that you don't really document, but rather express motivation for change, the documentation should belong in code. |
There was a problem hiding this comment.
Heh, I've been stressing out the whole week because I wanted to write down some of the things we talked about last meeting so that we could continue the discussion, and bam, I should've just looked on GH, you already did it ;)
So thank you, it will certainly help us in aligning our goals!
| windowing toolkit getting certain changes incorporated upstream could be | ||
| challenging. |
There was a problem hiding this comment.
Nit: "upstream" unnecessary here.
| windowing toolkit getting certain changes incorporated upstream could be | |
| challenging. | |
| windowing toolkit getting certain changes incorporated could be challenging. |
|
|
||
| To save your time it's wise to check already opened [pull requests][prs] and | ||
| [issues][issues]. In general, bug fixes and missing implementations are always | ||
| accepted, however new API proposals should go into the issue first. When in |
There was a problem hiding this comment.
I don't think you need to go through an issue if an API change is small.
| accepted, however new API proposals should go into the issue first. When in | |
| accepted, however larger new API proposals should go into the issue first. When in |
| - If you're a downstream user and your fixing your issue, consider to link it | ||
| like `Links: URL` in the end of the commit message. |
There was a problem hiding this comment.
This doesn't work that great on GitHub, as it clutters up the issue with a lot of backreferences if you're doing rebases. If the url is only in the GitHub PR, then it works fine.
There was a problem hiding this comment.
I know, but it's a problem of github in particular. Having correct links helps a lot when looking why the patch was send in the first place.
| The maintainers will fixup your commit message when applying the patch, but it's | ||
| still recommended to follow the rules above to make the life of maintainers | ||
| easier. |
There was a problem hiding this comment.
I already know this is gonna be difficult for me to remember to do, unless we add some tooling to help with it.
this option can't be used reliably by some parties
Relates to this, "some parties" is me ;)
| easier. | ||
|
|
||
| If you want to communicate something to reviewers, but it doesn't belong to | ||
| the commit message, write that under `--` on PR body. Your PR could look like: |
There was a problem hiding this comment.
Nit: Works better with markdown.
| the commit message, write that under `--` on PR body. Your PR could look like: | |
| the commit message, write that under `---` on PR body. Your PR could look like: |
There was a problem hiding this comment.
No the -- is a standard symbol for this. It's automatically added by the git itself when you author a patch for submission the traditional way.
There was a problem hiding this comment.
But you're writing that in the GitHub PR body, not in Git?
There was a problem hiding this comment.
yeah, that's true. I'm just really used to that notation and it's quite common.
| Winit has plenty of maintainers with different backgrounds, experience level, | ||
| and reasons to be winit maintainer in the first place. To ensure that winit's |
There was a problem hiding this comment.
Nit: While definitely true, I feel it's not really relevant to state, and it's more important state that we don't all have the same bandwidth.
| Winit has plenty of maintainers with different backgrounds, experience level, | |
| and reasons to be winit maintainer in the first place. To ensure that winit's | |
| Winit has plenty of maintainers with different backgrounds, different time available to work on Winit, | |
| and different reasons to be winit maintainer in the first place. To ensure that winit's |
| requiring contributor to match *maintainer* quality standards when writing | ||
| patches and commit messages. | ||
|
|
||
| ### Contributing |
There was a problem hiding this comment.
Nit: A bit odd that there's two headers called "Contributing". Maybe rename this to something else?
There was a problem hiding this comment.
If you have a better name sure, but it's a subsection in maintainers section extending on the contributors section.
| ### Contributions handling | ||
|
|
||
| The maintainer must ensure that the external contributions meet the winit's | ||
| quality standards. If it's not, it **is maintainer's responsibility** to bring |
There was a problem hiding this comment.
Nit: typo
| quality standards. If it's not, it **is maintainer's responsibility** to bring | |
| quality standards. If it's not, it **is the maintainer's responsibility** to bring |
| under the project repository instead of your own fork. The naming scheme is | ||
| `github_user_name/branch_name`. Doing so will make your work easier to rebase | ||
| for other maintainers when you're absent. | ||
|
|
There was a problem hiding this comment.
| ### Administrative actions | |
| Some things (such as changing required CI steps, adding contributors, ...) require administrative permissions. If you don't have those, ask about the change in an issue. If you have the permissions, discuss it with at least one other admin before making the change. | |
There was a problem hiding this comment.
I'm not sure it does? It requires you to be collaborator.
There was a problem hiding this comment.
There's a whole set of different GitHub "roles", but some things do require admin permissions, take a look at https://github.com/rust-windowing/winit/settings/access.
Organization-wide, there's "Owner" and "Member", and only the latter can add others to the org.
In any case, I mostly wanted to codify our previous decision about discussing administrative changes with others before doing them.
| - Decline the patch if it tries to achieve short term goals instead of solving | ||
| the problem in general. In such case maintainer must communicate that **as | ||
| early as possible**, so contributors don't spend their time on something | ||
| that won't be accepted in the end. |
There was a problem hiding this comment.
I acknowledge that this may lead to decreased code quality, but I believe that requiring the problem to be solved in general is the wrong approach for Winit. While the project is a few years old, it's still young enough that we don't yet know how to do everything! And the absolute best way to get knowledge is to let people experiment with the code!
As an example, #3499 is solving a short-term goal, but I believe it'll also help guide how the API will have to look like in the future.
Another example is the file open APIs on macOS, which got rejected partly because of technical difficulties in making it work right, but also because we wanted to find some solution to #2120, which has been open for over two years with only recently a solution on the horizon.
My opinion is that we need to get all of the sub-par code into Winit, so that we can iterate on it in there instead of in a PR that will have to be rebased over an over (keyboard v2).
There was a problem hiding this comment.
I acknowledge that this may lead to decreased code quality, but I believe that requiring the problem to be solved in general is the wrong approach for Winit. While the project is a few years old, it's still young enough that we don't yet know how to do everything! And the absolute best way to get knowledge is to let people experiment with the code!
it's like 10 years old at this point and it was inside glutin at some point.
As an example, #3499 is solving a short-term goal, but I believe it'll also help guide how the API will have to look like in the future.
This is not how the API will look though. The trait extension is fine, but it was not proposed there.
Another example is the file open APIs on macOS, which got rejected partly because of technical difficulties in making it work right, but also because we wanted to find some solution to #2120, which has been open for over two years with only recently a solution on the horizon.
Yes, the solution is merged and can be used to implement the said API already.
My opinion is that we need to get all of the sub-par code into Winit, so that we can iterate on it in there instead of in a PR that will have to be rebased over an over (keyboard v2).
No, the subpar code will stay with us forever, that's how it works unfortunately. Once you land it it'll be pretty much like that for a long time. There're exceptions, but it's how it is, unfortunately. You should never justify not maintainable code.
Give better recommendations for contributors on how to propose start the work, write commit messages, and handle review by explicitly stating expectations, however make it clear that maintainers will help you if you have issues.
Write down the guidelines for maintainers on how they should handle contributions and interact with other maintainers to help maintaining good code quality level by listing maintainers responsibilities and providing suggestions on how to handle various situations.