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

Turn mistakenly disabled linters back on #1142

Open
zenovich opened this issue Aug 5, 2024 · 0 comments
Open

Turn mistakenly disabled linters back on #1142

zenovich opened this issue Aug 5, 2024 · 0 comments
Assignees
Labels
Type: Maintenance Change required to improve the tech debt

Comments

@zenovich
Copy link
Collaborator

zenovich commented Aug 5, 2024

For an unknown (but probably guessable) reason, most of useful linters checking our code for issues were disabled in #936 (see https://github.com/France-ioi/AlgoreaBackend/pull/936/files#diff-6179837f7df53a6f05c522b6b7bb566d484d5465d9894fb04910dd08bb40dcc9R70).

The list of disabled linters looks like this:

    - forbidigo        # For update to go1.20
    - gci              # For update to go1.20
    - gochecknoinits   # For update to go1.20
    - revive           # For update to go1.20
    - asasalint        # For update to go1.20
    - containedctx     # For update to go1.20
    - contextcheck     # For update to go1.20
    - cyclop           # For update to go1.20
    - dupword          # For update to go1.20
    - revive           # For update to go1.20
    - errchkjson       # For update to go1.20
    - errorlint        # For update to go1.20
    - errname          # For update to go1.20
    - execinquery      # For update to go1.20
    - exhaustive       # For update to go1.20
    - exhaustivestruct # For update to go1.20
    - forcetypeassert  # For update to go1.20
    - ifshort          # For update to go1.20
    - ireturn          # For update to go1.20
    - ineffassign      # For update to go1.20
    - maintidx         # For update to go1.20
    - musttag          # For update to go1.20
    - nilnil           # For update to go1.20
    - nlreturn         # For update to go1.20
    - noctx            # For update to go1.20
    - nonamedreturns   # For update to go1.20
    - paralleltest     # For update to go1.20
    - sqlclosecheck    # For update to go1.20
    - tagliatelle      # For update to go1.20
    - tenv             # For update to go1.20
    - thelper          # For update to go1.20
    - usestdlibvars    # For update to go1.20
    - varnamelen       # For update to go1.20
    - wrapcheck        # For update to go1.20
    - exhaustruct      # For update to go1.20
    - gomoddirectives  # For update to go1.20
    - typecheck        # For update to go1.20
    - dupl             # For update to go1.20

I don't ask what "For update to go1.20" means, but we do really use go1.20 currently. Also, all these disabled linters support go1.20.

It's hard to list all the possible issues the linter ignores as of this change, but from what I've discovered already:

  1. golangci-lint allows creating public packages/methods/functions/variables/constants/types without a description (like here https://github.com/France-ioi/AlgoreaBackend/blame/master/app/database/group_store.go#L297).
  2. golangci-lint allows starting variable names with a capital letter (like here https://github.com/France-ioi/AlgoreaBackend/blame/master/app/database/group_store.go#L218).
  3. golangci-lint allows duplicated code blocks, i.e. writing code by copying and pasting instead of extracting methods.

Instead, #936 introduced a strange and unnecessary rule requiring all existing function/method/variable/type/package/constant comments to end with a dot, even if it is a two-word comment. We'd never had this rule in our coding standards.

We need to turn all the linters listed above on again to restore the code style and quality checking. Actually, the linter configuration was the coding standards of the project, which were fixed in the configuration of golangci-lint and could not have been changed implicitly. It's very similar to how the architecture decision about per-package code coverage was fixed in the Makefile. There is no need to write lengthy documents describing such coding standards and decisions, no need to duplicate the same thing in different places, DRY.

So, as this task is big as it requires to fix the code added after March 2023, I suggest enabling linters one by one.

I would start from 'revive' as it checks lots of things (see the list of rules here https://golangci-lint.run/usage/linters/#revive). It is done in #1143.

Next, I would enable dupl which searches for duplicates: #1144

@zenovich zenovich added the Type: Maintenance Change required to improve the tech debt label Aug 5, 2024
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Type: Maintenance Change required to improve the tech debt
Projects
None yet
Development

No branches or pull requests

1 participant