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

Make go vet happy #205

Merged
merged 1 commit into from
Jul 27, 2021
Merged

Make go vet happy #205

merged 1 commit into from
Jul 27, 2021

Conversation

rvagg
Copy link
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 warpfork and mvdan July 21, 2021 03:11
@mvdan
Copy link
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
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
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
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
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