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

doc/contribute: describe what changes (CLs) are not OK #28212

Open
quasilyte opened this issue Oct 15, 2018 · 12 comments

Comments

@quasilyte
Copy link
Contributor

commented Oct 15, 2018

Imagine this code:

x += 1

golint suggest to simplify it to x++.
Now given this code:

x = x + 1

No warning is generated. But it's still x++ and it's probably more consistent with most of Go code to write x++:

~/go $ gogrep -x '$x++' ./src/... | wc -l
5903
~/go $ gogrep -x '$x = $x + 1' ./src/... | wc -l
6

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 to x -= 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 some x 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.)

@quasilyte

This comment has been minimized.

Copy link
Contributor Author

commented Oct 15, 2018

It's the worst of two world if both reviewers and newcomers are upset during the process.
This is a two-side misunderstanding.
If we can at least partially fix this by some kind of rules or hints in the documentation (https://golang.org/doc/contribute.html), it could in theory help to both sides. Reviewer could reference the (potentially violated) rule, the contributor could choose what not to send based on those rules so the problem never arises.

@mvdan

This comment has been minimized.

Copy link
Member

commented Oct 15, 2018

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:

  • Many small, trivial CLs should generally be merged into one
  • It's fine for a single CL to cover many unrelated packages if the changes are very similar
  • The advantage of a change should outweigh the git noise (e.g. losing git blame, adding to git log)

I'm not sure if doc/contribute.html is a good place for this - we don't want that page to get too big, as we send new contributors there. Perhaps we could add this to the wiki somewhere.

@mvdan

This comment has been minimized.

Copy link
Member

commented Oct 15, 2018

Also /cc @rasky @andybons @dmitshur, for their involvement on the contribution guide and new contributors in general.

@FiloSottile FiloSottile added this to the Unplanned milestone Oct 15, 2018

@andybons

This comment has been minimized.

Copy link
Member

commented Oct 15, 2018

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.

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:

dst[3*i+0] = r
dst[3*i+1] = g
dst[3*i+2] = b

We made a stylistic decision to have the first line align with the others, when we all knew it could be simplified as dst[3*i] = r. It doesn't matter, though, the compiler outputs the same result (and if it didn't, then we fix the compiler). What would be the benefit to a CL changing it now?

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.

@rasky

This comment has been minimized.

Copy link
Member

commented Oct 15, 2018

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

@quasilyte

This comment has been minimized.

Copy link
Contributor Author

commented Oct 15, 2018

I think it is impossible to have accurate documentation of what kind of CL is acceptable and what it is not.

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 s[:] => s changes also fall into the same category?
There are also changes like !(x == y) => x != y. It also doesn't fix anything and doesn't bring new features or performance improvements. Hard to draw a line here. Personally, I think all of these do make code a little bit simpler and it's a bad practice to make unrelated changes when you work on something complicated (read: important), so spare hands can be useful here. They're also trivial to review, at least from my point of view. Since these changes don't require any context at all, they look like a perfect candidates for a first (or second) CL. This was my initial way of thinking. It's apparent now that it's not a popular one here.

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 x = x + 1 can be interpreted as x++.

The problem with s[:] (where s is a slice or a string) in particular is that some people, while reading Go code, think that it has any effect. It doesn't and is optimized away by the compiler.

So to reiterate, if you're unsure, just ask! We're here to answer any questions and provide justification for our reasoning.

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.

@ianlancetaylor

This comment has been minimized.

Copy link
Contributor

commented Oct 15, 2018

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 s[:] with s will make the code clearer. Whether replacing x = x + 1 with x++ makes the code clearer really depends on the code. Generally it is not clearer. Both forms are equally clear.

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 git blame less useful. Churn should be avoided.

@beoran

This comment has been minimized.

Copy link

commented Oct 16, 2018

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.

@quasilyte

This comment has been minimized.

Copy link
Contributor Author

commented Oct 16, 2018

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

@beoran

This comment has been minimized.

Copy link

commented Oct 16, 2018

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.

@mvdan

This comment has been minimized.

Copy link
Member

commented Oct 18, 2018

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 CodeSubmissionTips or GerritGuidelines.

@beoran

This comment has been minimized.

Copy link

commented Oct 19, 2018

Sure, a new page CodeSubmissionTips sounds like a good idea.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
7 participants
You can’t perform that action at this time.