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

Rework CONTRIBUTING guidelines #3549

Closed
wants to merge 1 commit into from

Conversation

kchibisov
Copy link
Member

@kchibisov kchibisov commented Mar 1, 2024

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.

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.
Copy link
Member

@notgull notgull left a comment

Choose a reason for hiding this comment

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

It should also be mentioned here that Markdown files should be wrapped to 80 characters.

#### Creating commits

When creating a commit, follow these general rules:
- Try to limit the first line (title) of the commit message to 50 characters,
Copy link
Member

Choose a reason for hiding this comment

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

I think that a link here to the Git standards this is taken from would be appreciated.

Copy link
Member Author

Choose a reason for hiding this comment

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

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

Copy link
Member

Choose a reason for hiding this comment

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

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
Copy link
Member

Choose a reason for hiding this comment

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

This paragraph seems unnecessary and overly restrictive, so some context would be appreciated.

Copy link
Member Author

Choose a reason for hiding this comment

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

I mean, it's a regular process of evolving. It's present in mostly every project I worked in.

Copy link
Member

Choose a reason for hiding this comment

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

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.

@kchibisov
Copy link
Member Author

It should also be mentioned here that Markdown files should be wrapped to 80 characters.

I was thinking on adding .editorconfig for that.

@notgull
Copy link
Member

notgull commented Mar 3, 2024

I was thinking on adding .editorconfig for that.

While this would be a good solution I would prefer it if it was stated explicitly here.

Copy link
Member

@daxpedda daxpedda left a comment

Choose a reason for hiding this comment

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

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.

@kchibisov
Copy link
Member Author

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.

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.

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

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.

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.

Well, it's the topic line, brevity is more important. If you need more than you likely need to split commit into multiples.

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.

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.

@daxpedda daxpedda added the C - nominated Nominated for discussion in the next meeting label Mar 5, 2024
Copy link
Member

@madsmtm madsmtm left a comment

Choose a reason for hiding this comment

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

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!

Comment on lines +9 to +10
windowing toolkit getting certain changes incorporated upstream could be
challenging.
Copy link
Member

Choose a reason for hiding this comment

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

Nit: "upstream" unnecessary here.

Suggested change
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
Copy link
Member

Choose a reason for hiding this comment

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

I don't think you need to go through an issue if an API change is small.

Suggested change
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

Comment on lines +35 to +36
- 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.
Copy link
Member

Choose a reason for hiding this comment

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

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.

Copy link
Member Author

Choose a reason for hiding this comment

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

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.

Comment on lines +47 to +49
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.
Copy link
Member

@madsmtm madsmtm Mar 8, 2024

Choose a reason for hiding this comment

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

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:
Copy link
Member

Choose a reason for hiding this comment

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

Nit: Works better with markdown.

Suggested change
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:

Copy link
Member Author

Choose a reason for hiding this comment

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

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.

Copy link
Member

Choose a reason for hiding this comment

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

But you're writing that in the GitHub PR body, not in Git?

Copy link
Member Author

Choose a reason for hiding this comment

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

yeah, that's true. I'm just really used to that notation and it's quite common.

Comment on lines +98 to +99
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
Copy link
Member

Choose a reason for hiding this comment

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

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.

Suggested change
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
Copy link
Member

Choose a reason for hiding this comment

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

Nit: A bit odd that there's two headers called "Contributing". Maybe rename this to something else?

Copy link
Member Author

Choose a reason for hiding this comment

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

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
Copy link
Member

Choose a reason for hiding this comment

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

Nit: typo

Suggested change
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.

Copy link
Member

Choose a reason for hiding this comment

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

Suggested change
### 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.

Copy link
Member Author

Choose a reason for hiding this comment

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

I'm not sure it does? It requires you to be collaborator.

Copy link
Member

Choose a reason for hiding this comment

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

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.

Comment on lines +112 to +115
- 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.
Copy link
Member

Choose a reason for hiding this comment

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

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

Copy link
Member Author

Choose a reason for hiding this comment

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

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.

Copy link
Member

@daxpedda daxpedda left a comment

Choose a reason for hiding this comment

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

We decided to split off any commit message rules into a separate PR and merge everything else now, after all nits are resolved.

@madsmtm madsmtm removed the C - nominated Nominated for discussion in the next meeting label Apr 12, 2024
@kchibisov kchibisov closed this Apr 26, 2024
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Development

Successfully merging this pull request may close these issues.

4 participants