-
Notifications
You must be signed in to change notification settings - Fork 17.9k
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
doc/contribute: describe what changes (CLs) are not OK #28212
Comments
It's the worst of two world if both reviewers and newcomers are upset during the process. |
I think it's good if we write down some of the common knowledge that some (most?) of the Go reviewers apply for these CLs. For example:
I'm not sure if |
If you're unsure of whether to pursue a cleanup change like the one you described and attempted, then emailing golang-dev@ will give you an answer before you send out a single CL. I agree with @mvdan to start with the three points he described on the wiki somewhere. We appreciate any attempt at contribution, but golang.org/cl/141957 is just churn. Blindly standardizing on something so minuscule that doesn't actively help or hurt readability of the code shouldn't be pursued. The costs (code reviews, lost git blame, etc.) far outweigh the benefits. There are differing scenarios where one would be justified in using one form over another. The following snippet is from the image/gif package:
We made a stylistic decision to have the first line align with the others, when we all knew it could be simplified as Regarding golint, there are many places within the Go source that don't abide by its standards. That's OK. The documentation for golint is very clear that the tool is meant as a guide and to not treat the output as a gold standard. So to reiterate, if you're unsure, just ask! We're here to answer any questions and provide justification for our reasoning. |
I think it is impossible to have accurate documentation of what kind of CL is acceptable and what it is not. If anything, it’s probably easier to just point to “help wanted” tag on GitHub as good first issues (some projects do also have a “good first issue” as well). |
I'm fine with no changes to the document. Just wanted to raise my concerns. Personally, I think a notice about "chorn" CLs and a definition of "chorn" is a good starting point. I believe that There are also typo fixes. They're also like a chorn in some sense, since the reader still can get an idea of the intention. Just like The problem with
This doesn't scale well. And even if I would ask, we can't be sure about someone who never really been involved into anything around Go. It would be exclusive-oriented tactic, not inclusive one. |
There are subjective elements to this. I don't think we can write down a rigorous classification. I would say that changes that make the code clearer are always OK. Typically replacing If a change does not make the code clearer, it should bring some other benefit. Simply changing the code from one form to another without increasing clarity is churn. Churn is bad. It can not help improve the code. It can only keep it the same or introduce a bug. It increases git history and makes |
I'd say it is the same as in many commercial environments: we should not make style only changes. Style has no value in itself, it is merely aesthetic. We should fix bugs or implement new features, then we can also improve the style, while we are at it. |
@beoran, you're right, of course, there is no double about concentrating on these things. But we're talking about contributors guide (which mostly targets new contributors) and it doesn't describe these values and this is a thing I'm trying to draw here. Implicit rules are not obvious (at least not for everyone). There are also different definitions of "contributors-friendly project". I do like to get involved into any project by sending one or two very minimal, mostly stylistic or typographical (most likely harmless things, so I don't break a build with my first PR) and only then try to grasp something complicated. It's a good way for me to filter out projects with maintainers that have very opinionated attitude, or projects that ignore new contributions (PRs stay open for years), for example. There are other people that follow this practice as well. And to give some context if it's missing: these problems arise when someone tries to organize Go contributors workshop like event. This is why I did create this issue, since I misunderstood almost everything about it. Even if I made my own conclusions, I would be glad if others can avoid some pitfalls. For example, we can state that sending a CL that merely applies some style linter suggestions is not (generally) a good idea and it can be rejected. |
I see what you say, but in general, for all open source projects, I would advise against making such a "testing the waters" contribution. It may be ignored because it might be judged as not having any real value to the project. I would try to find a small bug or feature you are willing to work on and contribute that instead. As for making it more clear for new contributors, it may be a good idea to mention what a good first contribution could be, and what not. |
Note that my proposed addition to the wiki is not a strict set of rules or a way to classify what a "good CL" is. The idea is to give some general advice and guidelines on how to write CLs so that they are more likely to be useful and merged. Different projects use git and code review in different ways, so it's understandable if they're surprised by the Go team's standards. In this specific case, I think that the three items I described above would have avoided the confusion that Iskander and his workshop attendees had. The new contributors would have either sent better CLs (in the eyes of maintainers), or looked for other changes to contribute. We already have https://github.com/golang/go/wiki/CodeReviewComments, so perhaps we could add |
Sure, a new page CodeSubmissionTips sounds like a good idea. |
Imagine this code:
golint suggest to simplify it to
x++
.Now given this code:
No warning is generated. But it's still
x++
and it's probably more consistent with most of Go code to writex++
:So, the
x++
is more idiomatic.The question is: is it a valid change for someone to send?
If yes, are there some other restrictions, like is it permitted to send 3 CLs that fix this issue in 3 packages or it's only OK to send 1 CL that fixes this issue in 3 packages at once? If yes, should the commit message include 3 package names separated by a comma or could it just say
all
?All these questions need to be answered.
Otherwise, it's going to be frustrating contributing experience for both newcomers and reviewers.
See CL141957 that made it apparent for me that I'm missing something from the picture. I remember that smallest changes do not require a gh issue or discussion prior to CL, but it's not that easy to filter what small changes are welcome and which ones are not.
And more interestingly, does this logic can be extended to
x = x - y
so it can be simplified tox -= y
? I thought that the answer is yes, but seems like it is not. This is not entirely subjective since there is a statistics that can be taken and the latter form is more frequent, shorter and in case of somex
leads to a single evaluation instead of 2 (wording taken from spec). There are no apparent benefits from the first form.The problem is that there are some (unwritten?) rules that make such small cleanup-like changes less likely to be merged. My proposal is to describe these rules and hopefully with some rationale behind it. It would be fair and honest to everyone.
There are several concerns, I agree, but they should be stated explicitly.
CC @mvdan @ianlancetaylor @bradfitz
(I'm not 100% sure who to CC here, please add anyone who can be interested in this. Thanks.)
The text was updated successfully, but these errors were encountered: