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.

Sign up for GitHub

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

Jump to bottom

gofmt -s and some staticcheck work #506

Closed
wants to merge 3 commits into from
Closed

gofmt -s and some staticcheck work #506

wants to merge 3 commits into from

Conversation

mvdan
Copy link
Contributor

@mvdan mvdan commented Dec 27, 2023

(see commit messages - please do not squash)

@mvdan
all: gofmt -s
e396742 
And enforce it via make.
@mvdan
Copy link
Contributor Author

mvdan commented Dec 27, 2023

FWIW I would definitely recommend following go vet ./... and staticcheck ./.... There's quite a bit of work to appease staticcheck, but from the sample I chose above, you can hopefully see it's worth it.

@bnewbold
Copy link
Collaborator

bnewbold commented Dec 27, 2023 •
edited

This one is probably a bit harder to merge as-is. Do really appreciate the contribution.

Some notes:

  • go vet ./... (vanilla instead of the current tweaked settings): I support this enthusiastically. @whyrusleeping wanted to skip some lints early on. I think there are really just a handful of structs where this comes in to play. maybe we can just cut over now?
  • staticcheck ./...: as a generally thing i'm in favor of more static analysis. getting towards golangci-lint would be great.
  • comatproto vs atproto package name: I'm not-so-secretly hoping to change the package names to api/comatproto and api/appbsky at some point, to align with the full lexicon names, and it will be an easier refactor if we keep the import aliases as they are. comatprototypes should go though.
  • removing all the explicit struct types, I assume via gofmt -s: seems like a style decision? I'm not sure.
  • some of the other "simplify" things, like shorter appends, are fine but lower-priority right now

There seem to be some real fixes in here w/r/t unhandled err and things like that, it would be nice to have bugfix commits (with behavior changes) in a separate PR from style and lint commits (no behavior changes).

@bnewbold
Copy link
Collaborator

By commit:

  • all: gofmt -s : I would probably pass on this for now
  • all: fix a number of staticcheck warnings: mixed bag. avoiding for-loops is good, most other things good; renaming imports I would mostly pass on for now
  • all: fix a number of unused values: this looks very valuable. I'd want another set of eyes on review for the bgs and carstore stuff, but would almost certainly merge

@mvdan
Copy link
Contributor Author

mvdan commented Dec 27, 2023

gofmt -s removes redundant syntax, and is a pretty standard thing from upstream Go. Some projects only enforce gofmt, but many enforce gofmt -s as well, and I honestly don't see a drawback. Up to you of course.

Renaming imports up to you - it sounds like you're in the middle of rethinking import paths. Removing that part.

Note that staticcheck and golangci-lint are pretty different tools :) The former is like an extension of vet, with high quality checks that aim for very low false positives. golangci-lint is a collection of linters with a big range of quality, speed, and purpose. I'd personally stick to gofmt+vet+staticcheck in general.

@mvdan
all: fix a number of staticcheck warnings
c213ba2 
All harmless: dropping unnecessary append loops and fmt.Sprintf calls,
simplifying boolean checks, and removing redundant types.
@mvdan
all: fix a number of unused values
b041f38 
cids in ImportSlice has been unused since 2023/09.

GetFollows built an "out" slice but never returned it,
which seems like a bug.

TransformPost already collects image alt texts in another loop.

A few places like testing.SetupBGS did not check error values.
Leave one in bgs as a TODO, since it's not clear to me
what kind of response should the server return.
@mvdan
Copy link
Contributor Author

mvdan commented Jan 15, 2024

@bnewbold any suggestion on next steps, before this gains merge conflicts?

@mvdan
Copy link
Contributor Author

mvdan commented Feb 9, 2024

Already gained some conflicts :) friendly bump, happy to solve them

@mvdan
Copy link
Contributor Author

mvdan commented May 25, 2024

A large amount of conflicts at this point. I'll leave it to you to figure out whether you want to obey these tools - I very much think you should.

@mvdan mvdan closed this May 25, 2024
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers
No reviews
Assignees
No one assigned
Labels
None yet
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.

None yet
2 participants
@mvdan @bnewbold