Update CONTRIBUTING.md #12714
Update CONTRIBUTING.md #12714
Conversation
|
You can test this PR using the following package version. |
|
Can we link this in a comment of the PR template and probably also the issue templates? something like: PR:
<!-- Is this your first PR? Please read the contribution guidelines found here: ... -->
Issue (Bug):
<!-- Do you consider to contribute in order to fix this issue? Please read the contribution guidelines found here: ... -->
Issue (Feature Request):
<!-- Do you consider to contribute in order to implement this feature? Please read the contribution guidelines found here: ... -->
<!-- Please let us know you want to contribute and wait for a team member to give you the "go" for it --> |
| - Rebase your changes to remove extraneous commits. Ideally the commit history should tell a clean story of how the PR was implemented (even though the process was probably not clean!) | ||
| - Provide meaningful commit comments | ||
| - **Do not** change code unrelated to the bug fix/feature | ||
| - **Do not** introduce spurious formatting or whitespace changes |
There was a problem hiding this comment.
I know we have differences of opinion here but I think this is too strict of a policy. It doesn't allow fixes to ever occur unless the entire file is replace. Code churn as you put it needs to happen.
IMO we need:
- An actual code formatting guide with enforced rules in an .editorconfig (has been needed for a while)
- Relax formatting changes like this so they are allowed if you notice something in the file you are already editing. Most projects allow fixes if you are already working in the file or its related to the feature you are working on.
- Clarify that formatting changes should only be of clear benefit: Modern syntax like
get { return ...toget => ..., remove blank lines with spaces, add space after // in comment, etc. These are unarguable for the most part. New devs will sometimes open PRs changing things that are unnecessary but that can be discussed case-by-case. I don't think it should be prohibited outright.
Finally, I get the impression you are a purist with git history. Maintaining a clear and easy to read history is most important to you. I've found its often better to just let the code evolve. It's rare that the full git history needs to be investigated. And when it does need to be investigated it can still be done even with formatting changes. It just requires a little more work. Bottom line I rank the cost/benefit different than you and prioritize code quality over git history quality.
There was a problem hiding this comment.
It's all logical and stuff, but you aren't the one who has to go through the file history when diagnosing issues because some random change has polluted git blame. We are doing it quite often and it's a rather annoying procedure.
And unlike preparing a PR it will have to be done forever while the code exists.
Another problem is reverting PRs that caused regressions. If PR was merged several months ago and it had formatting changes, there is a huge chance that it won't get reverted cleanly because the affected random pieces of code were already changed by other PRs. e. g. it was pretty much impossible to revert a change made a while ago which was rather frustrating and have cost me several hours of my life.
So I'd rather ignore the PR completely than increase the maintenance burden because of somebody's ideas of beauty.
There was a problem hiding this comment.
TL;DR: PR author is not the one who will have to maintain the code ad aeternam, so clean history is more important.
There was a problem hiding this comment.
because of somebody's ideas of beauty.
It's not beauty, it's code quality I'm advocating for. I think code quality is much more important than perfect git history. In fact I've never heard of someone who will refuse fixes because it will make the git history more complicated.
Again, I propose a few compromises;
- Only focus on clear improvements (which can be agreed ahead of time with an editorconfig rule set)
- Only allow formatting fixes for files already being edited for other reasons
You could also still allow formatting-only PRs that would fix stuff like this.
Several make commits without detailed review and a lot of formatting mistakes get made. I often see missed spaces after if for example in addition to what I stated above. It doesn't make sense not to allow fixes this. If you stand firm that these shouldn't be fixed I'll argue that we need to be much, much more careful during PR merges. In fact we should be running code formatting checks before allowing merge if you want to go this route.
maintain the code ad aeternam, so clean history is more important.
That logic doesn't seem to hold. The main reasons to go through the history are to find the original authors and ask them to take a look. Or to find the related PR's, issues and discussions to understand more background. As you already said, original authors come and go so there is a decreasing significance over time with git blame (significance approaches zero over project lifetime).
Concerning related PR's issues and discussions several projects provide a lot of comments and references in code to avoid having to dig through to find related issues and discussions. In the end PR is still useful to get to with git history though.
Edit:
Another thought: I don't think the idea of 'clean/perfect git history' meshes well with open source software development. Open source software development is inherently a bit more unstructured than a small team can manage. It is better to allow faster, more changes. Code churn will fix things over time and you shouldn't restrain it. The true power of community development is noticing the smaller issues too which seems to imply many PRs which will convolute git history anyway. (Most people will find the little things and what doesn't work, Few people will push forward new features).
There was a problem hiding this comment.
but you aren't the one who has to go through the file history when diagnosing issues because some random change has polluted git blame. We are doing it quite often and it's a rather annoying procedure.
Also, are you using git bisect for this? It can filter past a lot of commits quickly.
There was a problem hiding this comment.
These guidelines are aimed at improving the speed in which we can review PRs. You may have noticed that we have an ever-growing list of open PRs and not enough time to review them.
Not following these guidelines doesn't mean we close your PR, but it does mean that it's less likely to get reviewed in a timely manner. Personally, my brain gets very distracted by unrelated changes so much that I'll move onto some other urgent job instead of finishing reviewing.
As a concrete example, take a look at one of @MrJul's PRs such as #12765 and see how quickly you can review it. There's a good description, you can run the commit with the test before the fix to verify the problem and there are no extraneous changes. There's a reason that most of his PRs get merged quickly; they're a joy to review.
There was a problem hiding this comment.
Also, are you using git bisect for this? It can filter past a lot of commits quickly.
Yes, i use this almost daily and non-building or intermediate commits make it a terrible experience.
|
|
||
| * Add class A | ||
| * Add class B | ||
| **Features should be discussed with the core team before opening a PR.** Please open an issue to discuss the feature before starting work, to ensure that the core team are onboard. |
There was a problem hiding this comment.
I agree with this change, it's just that the team is often very busy and therefore doesn't respond to discussions, I wouldn't want this to lead to fewer contributions in the long run.
There was a problem hiding this comment.
True and not true. It also leads to frustration if a PR doesn't get merged in the end. So hard to get a good measure here.
There was a problem hiding this comment.
Yes, this is unfortunate, but I prefer contributors not to waste days or weeks of time on something which isn't going to be accepted anyway. Of course no-one is stopping people opening undiscussed PRs, but with this guidance maybe they'll know what they're getting into.
|
Ok, tweaked the wording a little based on conversations here and mentioned CONTRIBUTING.md in the PR template. |
👍 |
|
You can test this PR using the following package version. |
What does the pull request do?
Our
CONTRIBUTING.mdguide hasn't been substantially updated since it was created more than three years ago, and in that time the workload of the core team has greatly increased and we've learned some stuff about how to run a project (hopefully 🙂).This PR updates the
CONTRIBUTING.mdwith updated guidance for creating PRs in an attempt to guide contributors new and old to get their PRs accepted and merged as soon as possible.Feedback welcome!