Skip to content

Make go vet happy#205

Merged
warpfork merged 1 commit intomasterfrom
rvagg/vet
Jul 27, 2021
Merged

Make go vet happy#205
warpfork merged 1 commit intomasterfrom
rvagg/vet

Conversation

@rvagg
Copy link
Copy Markdown
Member

@rvagg rvagg commented Jul 21, 2021

@mvdan @warpfork I know you've had discussions about linting and whatnot and there's strongly held opinions about such things, and I'm wading into this without recollection of your discussions about the path forward (if any) here. It seemed to me that this was low-hanging fruit on the path toward a possible inclusion in the https://github.com/protocol/.github list. There's one minor codegen change in here which is going to add a few more lines to generated code.
Feel free to dismiss this! Not a big deal, just had some muscle memory doing this in a couple of other repos so I thought'd I'd try my hand while I was here.

@rvagg rvagg requested review from mvdan and warpfork July 21, 2021 03:11
@mvdan
Copy link
Copy Markdown
Contributor

mvdan commented Jul 21, 2021

I'm certainly in favor of making the standard checks happy :) I did put in most of the legwork, especially when it came to removing vet warnings in generated code. I didn't get it over the finish line as I just lost interest with the pushback: ipld/ipld#92 (comment)

I do think it would be unfortunate to have the same standard checks across PL, but not for one of the "flagship core" Go repos, so I think there's value in finishing this. I even helped file a proposal to make the check more lenient, in an attempt to reach some middle ground.

@rvagg
Copy link
Copy Markdown
Member Author

rvagg commented Jul 22, 2021

I've done more than my share of style-rage in JavaScript through the past so understand how rusted-on opinions can get when it comes to personal style.

There are a few fixes in here that I assume would unambiguously good, such as. the dead code. Having codegen be cleaner for users who are going to go vet their own code is probably a worthy goal too but also bumps up against the imposition of personal style.

It turns out that building a fmt into a language still doesn't solve these personal style things! There has to be just a single way to avoid that I guess!

@mvdan
Copy link
Copy Markdown
Contributor

mvdan commented Jul 22, 2021

Having codegen be cleaner for users who are going to go vet their own code is probably a worthy goal too but also bumps up against the imposition of personal style.

FYI Eric and I already agreed on making the generator vet-happy, to unblock the "unified CI" standard checks for other repos. I simply forgot to fix one or two remaining vet errors, because they only triggered in some schema edge cases.

Copy link
Copy Markdown
Collaborator

@warpfork warpfork left a comment

Choose a reason for hiding this comment

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

LGTM.

(I still don't care for the struct-initializers-always-explicit-field-names thing, but I've ranted about that before elsewhere, and am giving up on it as not really worth the debate.)

@warpfork
Copy link
Copy Markdown
Collaborator

I don't reckon this will conflict with anything else open or has been referenced by commit hash anywhere, so I'll just go ahead and rebase+merge this.

@warpfork warpfork merged commit 485aefa into master Jul 27, 2021
@warpfork warpfork deleted the rvagg/vet branch July 27, 2021 16:02
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

Successfully merging this pull request may close these issues.

3 participants