Skip to content

Conversation

@g4s8
Copy link

@g4s8 g4s8 commented Nov 19, 2024

Add new docenv linter to validate that struct fields with env tags are documented: https://github.com/g4s8/envdoc/blob/master/docenv/README.md

Linter source code: https://github.com/g4s8/envdoc/tree/master/linter

@boring-cyborg
Copy link

boring-cyborg bot commented Nov 19, 2024

Hey, thank you for opening your first Pull Request !

@CLAassistant
Copy link

CLAassistant commented Nov 19, 2024

CLA assistant check
All committers have signed the CLA.

@ldez ldez added the linter: new Support new linter label Nov 19, 2024
@ldez
Copy link
Member

ldez commented Nov 19, 2024

In order for a pull request adding a linter to be reviewed, the linter and the PR must follow some requirements.

  • The CLA must be signed

Pull Request Description

  • It must have a link to the linter repository.
  • It must provide a short description of the linter.

Linter

  • It must not be a duplicate of another linter or a rule of a linter (the team will help to verify that).
  • It must have a valid license (AGPL is not allowed), and the file must contain the required information by the license, ex: author, year, etc.
  • It must use Go version <= 1.22
  • The linter repository must have a CI and tests.
  • It must use go/analysis.
  • It must have a valid tag, ex: v1.0.0, v0.1.0.
  • It must not contain init().
  • It must not contain panic().
  • It must not contain log.Fatal(), os.Exit(), or similar.
  • It must not modify the AST.
  • It must not have false positives/negatives (the team will help to verify that).
  • It must have tests inside golangci-lint.

The Linter Tests Inside Golangci-lint

  • They must have at least one std lib import.
  • They must have integration tests without configuration (default).
  • They must have integration tests with configuration (if the linter has a configuration).

.golangci.next.reference.yml

  • The file .golangci.next.reference.yml must be updated.
  • The file .golangci.reference.yml must NOT be edited.
  • The linter must be added to the lists of available linters (alphabetical case-insensitive order).
    • enable and disable options
  • If the linter has a configuration, the exhaustive configuration of the linter must be added (alphabetical case-insensitive order)
    • The values must be different from the default ones.
    • The default values must be defined in a comment.
    • The option must have a short description.

Others Requirements

  • The files (tests and linter) inside golangci-lint must have the same name as the linter.
  • The .golangci.yml of golangci-lint itself must not be edited and the linter must not be added to this file.
  • The linters must be sorted in the alphabetical order (case-insensitive) in the lintersdb/builder_linter.go and .golangci.next.reference.yml.
  • The load mode (WithLoadMode(...)):
    • if the linter uses goanalysis.LoadModeSyntax -> no WithLoadForGoAnalysis() in lintersdb/builder_linter.go
    • if the linter uses goanalysis.LoadModeTypesInfo, it requires WithLoadForGoAnalysis() in lintersdb/builder_linter.go
  • The version in WithSince(...) must be the next minor version (v1.X.0) of golangci-lint.
  • WithURL() must contain the URL of the repository.

Recommendations

  • The file jsonschema/golangci.next.jsonschema.json should be updated.
  • The file jsonschema/golangci.jsonschema.json must NOT be edited.
  • The linter repository should have a readme and linting.
  • The linter should be published as a binary (useful to diagnose bug origins).
  • The linter repository should have a .gitignore (IDE files, binaries, OS files, etc. should not be committed)
  • A tag should never be recreated.
  • use main as the default branch name.

The golangci-lint team will edit this comment to check the boxes before and during the review.

The code review will start after the completion of those checkboxes (except for the specific items that the team will help to verify).

The reviews should be addressed as commits (no squash).

If the author of the PR is a member of the golangci-lint team, he should not edit this message.

This checklist does not imply that we will accept the linter.

@ldez
Copy link
Member

ldez commented Nov 19, 2024

Hello,

The checklist is not completed but I already have some problems with this linter:

  • the linter doesn't have its dedicated module (and its dedicated repo)
  • the linter is oriented to something specific and niche
  • the linter says it's related to env struct tag name but the option allows changing the name.
  • why structures with env struct tag should have more documentation than others?

Everything feels extremely niche and related to the promotion of your tool: envdoc to generate the comments on env struct tag.

I will sleep on that but, for now, I think this linter will not be accepted.

@ldez ldez added the blocked Need's direct action from maintainer label Nov 19, 2024
@ldez ldez self-requested a review November 19, 2024 22:26
@g4s8
Copy link
Author

g4s8 commented Nov 20, 2024

Hello @ldez, I see your point. The goal of both tools envdoc and the linter is to solve the problem of undocumented env vars, because I've seen this problem many times in my experience. The linter does no directly promote original tool, rather both tools "promotes" writing documentation for env vars.
I will understand if you decide that this linter is niche and close PR. If not, please let me know what should be fixed.

@alexandear
Copy link
Member

@g4s8 maybe it's better to create a doctag linter that will warn about any undocumented tags? This linter will be in a separate repository and not inside envdoc.

@g4s8
Copy link
Author

g4s8 commented Nov 20, 2024

@alexandear thanks, sounds like a good idea

@ldez
Copy link
Member

ldez commented Nov 20, 2024

Sorry, but a linter about "undocumented" tags will have the same problem: why fields with struct tags should be more documented than other fields?
This will lead to a large amount of false positives.

Based on my previous comment, for now, I closed this PR.

I recommend modifying your linter to be usable as a plugin: https://golangci-lint.run/contributing/new-linters/#how-to-add-a-private-linter-to-golangci-lint

@ldez ldez closed this Nov 20, 2024
@ldez ldez added declined and removed blocked Need's direct action from maintainer labels Nov 20, 2024
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

declined linter: new Support new linter

Projects

None yet

Development

Successfully merging this pull request may close these issues.

4 participants