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

add ability to set issue severity #1155

Merged
merged 10 commits into from May 25, 2020

Conversation

ryancurrah
Copy link
Member

@ryancurrah ryancurrah commented May 18, 2020

User facing changes:

  • added new severity config to allow for custom issue severity for out formats that support it based on severity rules. Case sensitive severity rules are supported as well
  • added severity support to checkstyle, code-climate and github out formats
  • pretty print checkstyle out format so humans can read it as well

Internal changes:

  • added a new field to Issue struct named Severity. This allows setting severity for issues
  • added a new method to Issue struct named Description. This allows reusing logic to create a issue description for printers. It's a combination of linter name and issue text
  • added a new method to Issue struct named Fingerprint. This allows reusing logic to create a issue md5 hash for printers. The fingerprint is made up of issue filename, text and if available the first line in the file
  • a new BaseRule struct has been added that allows us to extend the rule code beyond just using it for ExcludeRules
  • BaseRule methods have 100% test coverage
  • made NewRunner method easier to read by moving some logic out of it for creating new processors
  • made codeclimate code easier to read

@ryancurrah ryancurrah added the enhancement New feature or improvement label May 18, 2020
@ryancurrah
Copy link
Member Author

@jirfag or @ernado what are you thoughts on this feature? do you think it is a feasible way to solve this?

@ernado
Copy link
Member

ernado commented May 18, 2020

I have no objections, design looks good to me.

@ryancurrah
Copy link
Member Author

One thought I had where this could get complicated is if multiple out formats are supported. It would be difficult to change this to accommodate multiple out formats, atleast not without breaking changes.

@ernado
Copy link
Member

ernado commented May 23, 2020

cc: @jirfag

We can avoid breaking changes via keeping default behavior, so format will change only with explicit configuration.

So I'm pretty positive for this feature.

@ryancurrah
Copy link
Member Author

@ernado sounds fair. I will add some tests to this PR

@ryancurrah ryancurrah marked this pull request as ready for review May 24, 2020 05:20
@ryancurrah
Copy link
Member Author

Tests have been added

Copy link
Member

@jirfag jirfag left a comment

Choose a reason for hiding this comment

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

awesome work!
code duplication confuses a bit

pkg/config/config.go Outdated Show resolved Hide resolved
.golangci.example.yml Outdated Show resolved Hide resolved
pkg/lint/runner.go Outdated Show resolved Hide resolved
… rule types, moved severity config to it's own parent key named severity, reduced size of NewRunner function to make it easier to read
…mate reporter easier to read, checkstyle output is now pretty printed
@ryancurrah
Copy link
Member Author

@jirfag thanks for the review. Updated based on feedback and added more detail to the PR description

Copy link
Member

@jirfag jirfag left a comment

Choose a reason for hiding this comment

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

fantastic!

@ryancurrah ryancurrah merged commit fa7adcb into master May 25, 2020
@delete-merged-branch delete-merged-branch bot deleted the feature/127-add-severity-to-error-messages branch May 25, 2020 12:21
@emas80
Copy link

emas80 commented Jun 30, 2020

hi, any plans for releasing this?

@ldez ldez added this to the v1.28 milestone Mar 6, 2024
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
area: severity enhancement New feature or improvement
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

5 participants