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

chore(fmt): Enable disabled linters in global scope #238

Merged
merged 3 commits into from
Nov 11, 2021

Conversation

yitsushi
Copy link
Contributor

@yitsushi yitsushi commented Nov 9, 2021

What this PR does / why we need it:

Global options:

  • An explanation for all nolint annotations is required. Please do not just ignore it, tell us why is it ignored.

wsl

  • Fixed everywhere.

lll

  • Fix everywhere.
  • Added exception on https:// links.

gochecknoglobals

  • Fixed everywhere.

gci

  • Fixed everywhere (with golangci-lint run --fix)

godox

No TODO comments should live in the code without filed issues to track
them. The reason is simple: if we have a comment with "todo", it has the
same value as not having that comment at all, because no one will care
about it.

  • No TODO, BUG, and FIXME comments in the code.
  • Except if it has a filed GitHub reference.

Disabled linters: see commit message

Which issue(s) this PR fixes (optional, in fixes #<issue number>(, fixes #<issue_number>, ...) format, will close the issue(s) when PR gets merged):
Fixes #168

Special notes for your reviewer:

Removed some comments, because they are not relevant in the code yet. For example add CSI/CNI/RateLimit, we don't have a ticket about it, it's on a long term list, and when we add them, we can extend those structs (and that will basically part of the ticket), right now those comments had no value.

  • ADR: work in progress.

Checklist:

  • squashed commits into logical changes
  • includes documentation

@yitsushi yitsushi added the kind/cleanup Removing things previously overlooked label Nov 9, 2021
@codecov-commenter
Copy link

codecov-commenter commented Nov 9, 2021

Codecov Report

Merging #238 (3d17be0) into main (fe9bb9f) will decrease coverage by 15.48%.
The diff coverage is 43.20%.

❗ Current head 3d17be0 differs from pull request most recent head c46786b. Consider uploading reports for the commit c46786b to get more accurate results
Impacted file tree graph

@@             Coverage Diff             @@
##             main     #238       +/-   ##
===========================================
- Coverage   55.81%   40.33%   -15.49%     
===========================================
  Files          44       46        +2     
  Lines        2012     2162      +150     
===========================================
- Hits         1123      872      -251     
- Misses        781     1230      +449     
+ Partials      108       60       -48     
Impacted Files Coverage Δ
core/application/commands.go 71.66% <ø> (+2.70%) ⬆️
core/application/reconcile.go 0.00% <ø> (ø)
core/models/vmid.go 90.24% <ø> (ø)
core/models/volumes.go 0.00% <ø> (ø)
core/steps/microvm/delete.go 100.00% <ø> (ø)
core/steps/runtime/dir_create.go 73.07% <ø> (ø)
core/steps/runtime/dir_delete.go 81.81% <ø> (ø)
core/steps/runtime/initrd_mount.go 100.00% <ø> (ø)
core/steps/runtime/kernel_mount.go 100.00% <ø> (ø)
infrastructure/containerd/content.go 0.00% <0.00%> (-100.00%) ⬇️
... and 36 more

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update 6bcae78...c46786b. Read the comment docs.

@yitsushi yitsushi force-pushed the 168-linter branch 6 times, most recently from 0272eb0 to 4bc0506 Compare November 10, 2021 15:30
@yitsushi yitsushi marked this pull request as ready for review November 10, 2021 15:32
richardcase
richardcase previously approved these changes Nov 11, 2021
Copy link
Member

@richardcase richardcase left a comment

Choose a reason for hiding this comment

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

Just the 1 comment

.golangci.yml Outdated
- path: test/e2e/
linters:
- goerr113
- gomnd

linters:
disable-all: true
Copy link
Member

Choose a reason for hiding this comment

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

Should we flip this and set enable-all: true so we pick up new linters in the future? And then document any we explicitly disable

Copy link
Contributor Author

Choose a reason for hiding this comment

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

enable-all: true is deprecated

Copy link
Contributor Author

Choose a reason for hiding this comment

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

enabled all and disabled a selection of linters.

richardcase
richardcase previously approved these changes Nov 11, 2021
Global options:
* An explanation for all nolint annotations is required.
  Please do not just ignore it, tell us why is it ignored.

== wsl

* Fixed everywhere.

== lll

* Fix everywhere.
* Added exception on `https://` links.

== gochecknoglobals

* Fixed everywhere.

== gci

* Fixed everywhere (with `golangci-lint run --fix`)

== godox

No TODO comments should live in the code without filed issues to track
them. The reason is simple: if we have a comment with "todo", it has the
same value as not having that comment at all, because no one will care
about it.

* No TODO, BUG, and FIXME comments in the code.
* Except if it has a filed github issue reference.
Disabled linters:

golint:
  The linter 'golint' is deprecated (since v1.41.0) due to: The
  repository of the linter has been archived by the owner.  Replaced by
  revive.

interfacer:
  The linter 'interfacer' is deprecated (since v1.38.0) due to: The
  repository of the linter has been archived by the owner.

scopelint:
  The linter 'scopelint' is deprecated (since v1.39.0) due to: The
  repository of the linter has been deprecated by the owner.  Replaced
  by exportloopref.

maligned:
  fieldalignment

tagliatelle:
  We have a yaml structure already and it uses snake_case keys, this
  linter wants us to replace with camelCase. That would require a design
  document and update all yaml outputs.

ireturn:
  Question?

nilnil:
  We are using nil, nil returns in case it was not an error, but the
  resource does not exist. Later we might want to return an error and
  check for error type on the caller side.

exhaustivestruct:
  We don't want to specify all fields with nil and empty string. I see
  the point, if you specify and the default behavior is changed, it does
  not break the code.
@yitsushi yitsushi merged commit 2c7797d into liquidmetal-dev:main Nov 11, 2021
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
kind/cleanup Removing things previously overlooked
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Enable all the disabled linters in global scope
3 participants