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

split up snap package, remove unuseds, lint #28

Merged
merged 3 commits into from
Feb 16, 2024
Merged

Conversation

roelarents
Copy link
Contributor

Omschrijving

split up snap package, remove unuseds, lint

Type verandering

  • Refactor

Checklist:

  • Ik heb de code in deze PR zelf nogmaals nagekeken
  • Ik heb mijn code beter achtergelaten dan dat ik het aantrof
  • De code is leesbaar en de moeilijke onderdelen zijn voorzien van commentaar
  • Ik heb de tests toegevoegd/uitgebreid indien nodig
  • Ik heb de tests gedraaid die de werking van mijn wijziging bewijst
  • De PDOK documentatie is bijgewerkt indien nodig.
  • Er zit geen gevoelig informatie in deze PR (wachtwoorden etc)

Co-authored-by: Michiel Korpel <michiel.korpel@kadaster.nl>
mapslicehelp/mapslicehelp.go Show resolved Hide resolved
.golangci.yaml Outdated
@@ -81,7 +81,6 @@ linters:
- nilerr # finds the code that returns nil even if it checks that the error is not nil
- nolintlint # reports ill-formed or insufficient nolint directives
- nosprintfhostport # checks for misuse of Sprintf to construct a host with port in a URL
- perfsprint # Golang linter for performance, aiming at usages of fmt.Sprintf which have faster alternatives
Copy link
Collaborator

Choose a reason for hiding this comment

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

Zie je een issue met deze linter?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

ja ik vond het belangrijker/mooier/overzichtelijker om alles consistent te doen, ook als er geen variabelen zijn, dan te mixen. de performance winst lijkt me wel mee vallen.

mixen:
errors met fmt.errorf vs errors.new
string met fmt.sprintf("foo") gewoon "foo"

@kad-korpem join the discussion :)

Copy link
Collaborator

Choose a reason for hiding this comment

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

Hmm, ja. Voor GoKoala heb ik het uiteindelijk gewoon wel volgens advies van de linter opgelost. Het heeft wat impact op consistentie, maar het is uiteindelijk ook niet echt logisch om een string formatter te gebruiken als je een normale string gebruikt.
Enige recommendation van perfsprint die ik niet fantastisch mooi vind, is om iets als greeting := fmt.Sprintf("hello, %s", name) te vervangen door greeting := "hello, " + name. Maar als dat (in ieder geval in theorie) net zo goed of beter performt, kan ik er wel mee leven.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

@rkettelerij hak jij de knoop door? mijn mening is niet sterk

Copy link
Collaborator

Choose a reason for hiding this comment

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

Punt van consistentie snap ik wel, en is wel terecht. Aan de andere kant de linter helpt je wel snel om dit aan te wijzen (linter werkt ook in de IDE). Zou het erin laten, scheelt ook weer overal linter config aanpassen.

mathhelp/mathhelp.go Outdated Show resolved Hide resolved
Base automatically changed from areyouinorout to master February 16, 2024 09:52
@roelarents roelarents merged commit ba3abcb into master Feb 16, 2024
2 checks passed
@roelarents roelarents deleted the split-clean-lint branch February 16, 2024 10:53
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