chore(ci): add golangci-lint action and apply linting changes #120

clok · 2021-03-15T15:48:20Z

What does this do / why do we need it?

This adds linting and formatting expectations to the project. Linting allows for code written and maintained by a group of individuals to read as if it comes from one collective mind. This helps with maintainability, readability and other "bility" words.

How this PR fixes the problem?

It adds the golangci-lint project as a Development and CI dependency. It uses the linters that are enabled by default as well as:

What should your reviewer look out for in this PR?

These are linters that I have used in the past. I have found them helpful, but I would like the opinions of others.

Each commit contains the delta required to pass a given linter.

Check lists

Test passed
Coding style (indentation, etc)

Additional Comments (if any)

For reference, here was the golangci-lint output before these changes:

Which issue(s) does this PR fix?

fixes #89

coveralls · 2021-03-15T15:49:24Z

Pull Request Test Coverage Report for Build 658991180

44 of 61 (72.13%) changed or added relevant lines in 14 files are covered.
1 unchanged line in 1 file lost coverage.
Overall coverage increased (+0.6%) to 76.13%

Changes Missing Coverage	Covered Lines	Changed/Added Lines	%
commit_parser.go	5	6	83.33%
tag_reader.go	0	1	0.0%
tag_selector.go	7	8	87.5%
chglog.go	3	5	60.0%
cmd/git-chglog/config_loader.go	0	2	0.0%
cmd/git-chglog/questioner.go	0	4	0.0%
cmd/git-chglog/main.go	0	6	0.0%

Files with Coverage Reduction	New Missed Lines	%
tag_selector.go	1	90.63%

Totals
Change from base Build 652113868:	0.6%
Covered Lines:	1751
Relevant Lines:	2300

💛 - Coveralls

clok · 2021-03-15T16:02:33Z

fields.go

+	Type        string     // (e.g. `feat`)
+	Scope       string     // (e.g. `core`)
+	Subject     string     // (e.g. `Add new feature`)
+	JiraIssueID string     // (e.g. `RNWY-310`)


This is a BREAKING CHANGE as the original role out of the Jira integration expected JiraIssueId as the type in the config.

khos2ow · 2021-03-15T18:33:54Z

Thank you @clok for taking the initiative! I personally would like to suggest even more enhancement (collapsed for brevity).

content of .golangci.yml file.

# https://golangci-lint.run/usage/configuration/
run:
  timeout: 10m
  deadline: 5m

  tests: true

output:
  format: tab

linters-settings:
  govet:
    # report about shadowed variables
    check-shadowing: true

  golint:
    # minimal confidence for issues, default is 0.8
    min-confidence: 0.8

  gofmt:
    # simplify code: gofmt with `-s` option, true by default
    simplify: true

  goimports:
    # put imports beginning with prefix after 3rd-party packages;
    # it's a comma-separated list of prefixes
    local-prefixes: github.com/git-chglog/git-chglog

  gocyclo:
    # minimal code complexity to report, 30 by default (but we recommend 10-20)
    min-complexity: 10

  maligned:
    # print struct with more effective memory layout or not, false by default
    suggest-new: true

  dupl:
    # tokens count to trigger issue, 150 by default
    threshold: 100

  goconst:
    # minimal length of string constant, 3 by default
    min-len: 3
    # minimal occurrences count to trigger, 3 by default
    min-occurrences: 5

  lll:
    # tab width in spaces. Default to 1.
    tab-width: 1

  unused:
    # treat code as a program (not a library) and report unused exported identifiers; default is false.
    # XXX: if you enable this setting, unused will report a lot of false-positives in text editors:
    # if it's called for subdir of a project it can't find funcs usages. All text editor integrations
    # with golangci-lint call it on a directory with the changed file.
    check-exported: false

  unparam:
    # Inspect exported functions, default is false. Set to true if no external program/library imports your code.
    # XXX: if you enable this setting, unparam will report a lot of false-positives in text editors:
    # if it's called for subdir of a project it can't find external interfaces. All text editor integrations
    # with golangci-lint call it on a directory with the changed file.
    check-exported: false

  nakedret:
    # make an issue if func has more lines of code than this setting and it has naked returns; default is 30
    max-func-lines: 30

  prealloc:
    # XXX: we don't recommend using this linter before doing performance profiling.
    # For most programs usage of prealloc will be a premature optimization.

    # Report preallocation suggestions only on simple loops that have no returns/breaks/continues/gotos in them.
    # True by default.
    simple: true
    range-loops: true # Report preallocation suggestions on range loops, true by default
    for-loops: false # Report preallocation suggestions on for loops, false by default

  gocritic:
    # Enable multiple checks by tags, run `GL_DEBUG=gocritic golangci-lint` run to see all tags and checks.
    # Empty list by default. See https://github.com/go-critic/go-critic#usage -> section "Tags".
    enabled-tags:
      - performance

    settings: # settings passed to gocritic
      captLocal: # must be valid enabled check name
        paramsOnly: true
      rangeValCopy:
        sizeThreshold: 32

  misspell:
    locale: US

linters:
  enable:
    - megacheck
    - govet
    - gocyclo
    - gocritic
    - interfacer
    - goconst
    - goimports
    - gofmt  # We enable this as well as goimports for its simplify mode.
    - prealloc
    - golint
    - unconvert
    - misspell
    - nakedret
    - dupl
    - depguard

  presets:
    - bugs
    - unused
  fast: false

issues:
  # Excluding configuration per-path and per-linter
  exclude-rules:
    # Exclude some linters from running on tests files.
    - path: _test(ing)?\.go
      linters:
        - gocyclo
        - errcheck
        - dupl
        - gosec
        - scopelint
        - unparam

    # These are performance optimisations rather than style issues per se.
    # They warn when function arguments or range values copy a lot of memory
    # rather than using a pointer.
    - text: "(hugeParam|rangeValCopy):"
      linters:
      - gocritic

  # Independently from option `exclude` we use default exclude patterns,
  # it can be disabled by this option. To list all
  # excluded by default patterns execute `golangci-lint run --help`.
  # Default value for this option is true.
  exclude-use-default: false

  # Show only new issues: if there are unstaged changes or untracked files,
  # only those changes are analyzed, else only changes in HEAD~ are analyzed.
  # It's a super-useful option for integration of golangci-lint into existing
  # large codebase. It's not practical to fix all existing issues at the moment
  # of integration: much better don't allow issues in new code.
  # Default is false.
  new: false

  # Maximum issues count per one linter. Set to 0 to disable. Default is 50.
  max-per-linter: 0

  # Maximum count of issues with the same text. Set to 0 to disable. Default is 3.
  max-same-issues: 0

khos2ow · 2021-03-15T18:34:59Z

And please don't hesitate to let me know to give a hand for fixing these. I didn't already do it to prevent stepping on your toes.

clok · 2021-03-15T18:44:55Z

And please don't hesitate to let me know to give a hand for fixing these. I didn't already do it to prevent stepping on your toes.

@khos2ow Appreciate it! Just an FYI - no need to worry about stepping on my toes. Always appreciate a collaborative effort on coding. I just recommend communicating one's intentions to the team just so that way we don't have duplicated efforts.

I really appreciate the more detailed and thorough configuration. I am going to take some time to look it over. For now, if you want to take a pass at addressing these issues, please do so. I won't be able to focus on this until later this evening.

Thank you again for the collaboration! I'll keep an eye for any changes that may be pushed before taking on any other actions when I come back around to this.

khos2ow · 2021-03-15T18:57:44Z

Awesome, then I will commit the configuration just now, and will attempt to fix as much as I can a little while later.

Signed-off-by: Khosrow Moossavi <khos2ow@gmail.com>

khos2ow · 2021-03-15T19:00:10Z

I also removed myself from reviewers, as it doesn't make sense for me to be reviewing my own work! 😅

Signed-off-by: Khosrow Moossavi <khos2ow@gmail.com>

clok · 2021-03-16T04:12:27Z

@khos2ow thank you for the updates. I pushed a few other patches to address scopelint, gosec and unparam.

The prealloc linter in nice. The failure that is popping up is a bit interesting in this case. I don't think we will have the ability to know the size of the []*Tag slice that we will be building in that function. I am interested in what you think.

We still have gocyclo, errorlint and govet failures as well.

Signed-off-by: Khosrow Moossavi <khos2ow@gmail.com>

khos2ow · 2021-03-16T13:35:33Z

@clok I think the best approach for dealing with gocyclo issues was to ignore them. Basically the idea of having a complexity goal enforces us to not put everything in one function, but in some cases that's inevitable (e.g. commitFilter()).

On the other hand sortCommitGroups() has some potential for reducing the complexity, which in the scope of this PR, IMO, can be ignored and be tackled later on.

mavogel

Awesome!
I agree the gocyclo can be tackled later in separate issues. Feel free to open them, so we keep track. The BC is also fine. As the naming will be caught in the future and we are still pre-major.

clok · 2021-03-16T19:09:09Z

@khos2ow I like that approach. Thank you for putting the comments and TODO callouts. Agree that a separate slice of work can take care of those refactors.

Unless you feel we can take care of the prealloc failure, I am inclined to ignore it on that line. For now, I am going to add a comment in code and push that change.

@mavogel Roger that on the BC.

… now

khos2ow · 2021-03-16T20:27:40Z

chglog.go

+		scopeErr := back()
+		if scopeErr != nil {
+			log.Fatal(scopeErr)


This also can be written as

if err = back(); err != nil { log.Fatal(err) }

I like that. I always forget the inline boolean check.

clok added 8 commits March 15, 2021 09:42

chore(ci): add golangci-lint action

c8ebe45

chore(lint): address errcheck lint failures

73d2e59

chore(lint): address misspell lint failures

ac93dd6

< 8000 img data-test-selector="commits-avatar-stack-avatar-image" src="https://avatars.githubusercontent.com/u/1429775?s=40&v=4" width="20" height="20" alt="@clok" class=" avatar-user" />

chore(lint): address gocritic lint failures

f647c87

chore(lint): address golint lint failures

3a64855

chore(lint): address structcheck lint failures

d3da694

chore(lint): address gosimple lint failures

0fdf06e

chore(lint): address gofmt lint failures

91106c5

clok added the type: refactoring Refactoring source code or architecture label Mar 15, 2021

clok requested review from khos2ow and mavogel March 15, 2021 15:48

clok self-assigned this Mar 15, 2021

chore(ci): port to official golangci-lint github action

6edb46e

clok commented Mar 15, 2021

View reviewed changes

clok added the type: breaking change Has breaking changes label Mar 15, 2021

Update golangci configuration for better coverage

1036590

Signed-off-by: Khosrow Moossavi <khos2ow@gmail.com>

khos2ow removed their request for review March 15, 2021 18:59

khos2ow and others added 5 commits March 15, 2021 16:02

fix: file is not goimports-ed

a337f7b

Signed-off-by: Khosrow Moossavi <khos2ow@gmail.com>

fix: golint and exported functions comments

b552eee

Signed-off-by: Khosrow Moossavi <khos2ow@gmail.com>

chore(lint): address gosec G304 warning

9e3c792

chore(lint): address uparam warnings

cf4c4fd

chore(lint): address scopelint lint failures

7f73b03

clok requested a review from wadackel March 16, 2021 04:19

clok requested a review from ghostsquad March 16, 2021 04:19

fix: cyclomatic complexity

9cd8441

Signed-off-by: Khosrow Moossavi <khos2ow@gmail.com>

khos2ow force-pushed the ci/lint branch from 426ed2e to 9cd8441 Compare March 16, 2021 13:36

mavogel approved these changes Mar 16, 2021

View reviewed changes

clok added 2 commits March 16, 2021 14:19

chore(lint): address prealloc warning, noting that we are warning for…

b45d234

… now

chore(lint): address govet and errorlint failures

573faea

khos2ow reviewed Mar 16, 2021

View reviewed changes

chore: clean up defer logic when checking errors

d931bfe

clok merged commit ae3382b into master Mar 17, 2021

clok deleted the ci/lint branch March 17, 2021 02:24

khos2ow mentioned this pull request Mar 22, 2021

Proposal: drop AppVoyer in favor of GitHub Action #126

Closed

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

chore(ci): add golangci-lint action and apply linting changes #120

chore(ci): add golangci-lint action and apply linting changes #120

chore(ci): add golangci-lint action and apply linting changes #120

chore(ci): add golangci-lint action and apply linting changes #120

Conversation

What does this do / why do we need it?

How this PR fixes the problem?

What should your reviewer look out for in this PR?

Check lists

Additional Comments (if any)

Which issue(s) does this PR fix?

Pull Request Test Coverage Report for Build 658991180

💛 - Coveralls

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment