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

This might not always be welcome #2

Open
gdamore opened this issue Jun 17, 2018 · 3 comments
Open

This might not always be welcome #2

gdamore opened this issue Jun 17, 2018 · 3 comments

Comments

@gdamore
Copy link

gdamore commented Jun 17, 2018

gofmt is somewhat opinionated, and reasonable people can have differing opinions.

I generally use gofmt myself, but different versions of go fmt over the years have changed how they format code, and advice from the go maintainers has been not to require this in git hooks for example, because the results might not be stable, particularly when people use different versions of go. Sweeping automation here isn't really appropriate IMO.

The other thing I don't really like is that you're going to touch a bunch of repositories only with automation, falsely inflating your "contributions" on github, making you look a lot more productive than you really are.

The idea of "drive by" submitting PRs against a bunch of projects that you have no former affiliation with, and which are only based upon being in Golang, feels kind of slimy to me.

In my particular case, I noticed that your version of go fmt unwrapped lines which causes them to extend beyond 80 columns. I don't like to go beyond 80 columns, so I have carefully chosen to wrap and indent. This is permitted by the Go style guide, but go fmt undoes that.

@whilei
Copy link
Member

whilei commented Jun 18, 2018

Hi @gdamore, Thanks for voicing your concern.

I'm going to address your points in order of importance.

Is it slimy?

[...] feels kind of slimy to me.

I can assure you, there is nothing "slimy" about this project.

Here's why:

You're here. You knew where to come, and here I am too. In fact, I invited you
in my PR to come here and do exactly this - voice any concerns or annoyances.
I could have run this from behind closed doors with a "ghost" account, and we wouldn't
be having this conversation; that might have been slimy. Slimy means dodging
accountability or responsibility, and usually it involves sacrificing
integrity for selfish reasons. I can assure you that is not going on.

I'm developing incrementally and with attention to feedback, using all means available to me to make this project as positive and helpful as possible.

This project is in its infancy, and I have only run the program in intermittent
small batches. In fact, I haven't run the program for more than hour in total.
After each batch, I supervise the resulting PRs to get feedback on if it's creating a positive
impact, and what I can do to improve. I'm following up with real people as a real person.
Furthermore, I've been dealing politely with the robots asking me to sign CLAs, reformat
my commit messages, and read code coverage reports.

With this feedback, I'm eager and able to improve the tool and my configuration.

One of the first and most critical rules about this project is to never create duplicate PRs.
As you'll read over on #1, I'm deliberately and ongoing-ly working to avoid that at all costs.

I'm also improving my (and the default) blacklist to exclude vendor files, automatically-generated files, only-EOL changes, and more, I'm sure, to come.

Automated PRs

The idea of "drive by" submitting PRs against a bunch of projects that you have no former affiliation with, and which are only based upon being in Golang, [...]

Robots are ubiquitous, and drive-by's are common place.

As mentioned above, git style bots, CLA bots, code cov bots, even, and not least, Github automated security alerts are robots on Github "doing drive-bys."

In my mind, a bot vetting my git commit message style on any given PR's commits is equivalently doing a "drive-by." I did not ask to be vetted; simply by participating creatively in the ecosystem I'm going to have to deal with them. If repo maintainers are "allowed" to do or enable drive-by's on PR and Issue contributions, then it's only reasonable that they can deal with drive-by's from the receiving end, too.

Also, I'm working on establishing a clear and publicized pattern for repo maintainers to be able to opt-out. This might look like including NOFMT anywhere in a repo's code of conduct or README, and/or starting an issue on this repo where maintainers can simply add their repo to a "global blacklist."

This might not always be welcome

This might not always be welcome

You're right. There is no mode or product of creativity that is always welcome.

Managing ignorant, lazy, duplicate, wrong, and/or otherwise poor-quality contributions is par for the course in the life of a Github public repo maintainer. Even high quality changes are not always welcome, since there can be differing opinions on architecture, priorities, and style.

But, in the case of this project...

Usually, it is welcome.

Of 55 closed PRs (from 155 I've made), 40 of them were merged and 15 were discarded. That's a 72% "success" rate, and this tally includes PRs made from before I had thought about excluding auto-generated files and vendor dirs.

More importantly, and to reiterate: it's an experiment.

If it stops being mostly useful/welcome, then obviously I won't do it.

The opinion of gofmt

gofmt is somewhat opinionated, and reasonable people can have differing opinions.

I generally use gofmt myself, but different versions of go fmt over the years have changed how they format code, and advice from the go maintainers has been not to require this in git hooks for example, because the results might not be stable, particularly when people use different versions of go. Sweeping automation here isn't really appropriate IMO.

From https://blog.golang.org/go-fmt-your-code:

### Format your code

We recently conducted a survey of Go packages in the wild and found that about 70% 
of them are formatted according to gofmt's rules. This was more than expected - and 
thanks to everyone who uses gofmt - but it would be great to close the gap.

To format your code, you can use the gofmt tool directly:

    gofmt -w yourcode.go

Or you can use the "go fmt" command:

    go fmt path/to/your/package

To help keep your code in the canonical style, the Go repository contains hooks for
editors and version control systems that make it easy to run gofmt on your code.

For Vim users, the Vim plugin for Go includes the :Fmt command that runs gofmt on the current buffer.

For emacs users, go-mode.el provides a gofmt-before-save hook that can be installed 
by adding this line to your .emacs file:

    (add-hook 'before-save-hook #'gofmt-before-save)

For Eclipse or Sublime Text users, the GoClipse and GoSublime projects add a 
gofmt facility to those editors.

And for Git aficionados, the misc/git/pre-commit script is a pre-commit hook that prevents
incorrectly-formatted Go code from being committed. If you use Mercurial, 
the hgstyle plugin provides a gofmt pre-commit hook.

So as far as I can tell, gofmt isn't "opinionated" -- though you're right, it is somewhat unstable and can change version by version. In fact, I learned this from one of the early contributions of the project to golang/go itelf.

I've since upgraded my Go to the latest stable 1.10.3, and include this information in my commit message.

The little green squares

The other thing I don't really like is that you're going to touch a bunch of repositories only with automation, falsely inflating your "contributions" on github, making you look a lot more productive than you really are.

I'm not really sure where to start with this one. ...I had started writing down this path at several points, and realize now that it's probably just like a whole Weltanschauung type thing. I'm going to leave a few of the paragraphs below in because:

  • one of them contains the word mischeviously, which doesn't get enough use
  • one or two are moderately hilarious
  1. There's the counting "gold stars" angle.

Those little green squares reflect the results of an arbitrary accounting mechanism devised by Github to, as far as a I can tell, either fill space on profile pages and show off how good they are at Javascript and CSS, or monger Github activity and "artificially inflate" their own daily active user count by exploting a human need to be recognized for effort... sublty, mischeviously, and greedily implying that human creativity and effort can be quantified by some joins on ActiveRecord.

This one was going to be a fun one because I was going to get all into how there's so much stuff that I've "produced" that Github is just plain missing out on, like that time I fixed my neighbor's bike, that basil plant I grew in my window once, the poem I wrote in 6th grade English class, and how I was actually, with this tool, un-deflating my true productivity score.

Then I was going to jump into a few peer-review examples, just for context and references.

For example, this guy submits his first Github issue from an unused Github account, reporting having "accidentally" caused more than 200 million USD in loss. Either a pretty big "productivity" score (even if it is in the red), and only a single light-green square on a Tuesday. And now the account is deleted, so there's even fewer little green squares.

Another example, famous Github user sindresorhus has, with 4k+ green points this year, duped a whole generation into thinking that he's some kind of wildly-prolific, inventive, generous, and dedicated "open-sourcer" and code writer. Little does everyone know, he's actually writing mostly Javascript.

  1. And then there's the whole thing about private repos, but that's just kind of boringly obvious.

  2. There's the robot/automation angle. The farmer doesn't deserve his crops because because he uses a tractor instead of an ox and plow.

  3. Finally, there's the whole "it's an open source program and anyone can do it too" perspective.

I digress.


I bet this is more than you bargained for, but I think it's important for this project and precedence to establish a clear context.

I haven't meant to offend, and hope I haven't, and I apologize for proposing to unwrap your lines.

@whilei
Copy link
Member

whilei commented Jun 18, 2018

An update: I've just been made aware of https://golang.org/doc/go1.10#gofmt ...

Note that these kinds of minor updates to gofmt are expected from time to time. In general, we recommend against building systems that check that source code matches the output of a specific version of gofmt. For example, a continuous integration test that fails if any code already checked into a repository is not “properly formatted” is inherently fragile and not recommended.

If multiple programs must agree about which version of gofmt is used to format a source file, we recommend that they do this by arranging to invoke the same gofmt binary. For example, in the Go open source repository, our Git pre-commit hook is written in Go and could import go/format directly, but instead it invokes the gofmt binary found in the current path, so that the pre-commit hook need not be recompiled each time gofmt changes.

So that adds a little extra flavor to the gofmt opinion question.

Maybe the best workaround for this is to run a check against a bunch of versions of gofmt, and proceed with proposing changes IFF all versions present significant changes.

@gdamore
Copy link
Author

gdamore commented Jun 18, 2018 via email

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

No branches or pull requests

2 participants