Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
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
refactor: replace panic with error in rules #1126
base: master
Are you sure you want to change the base?
refactor: replace panic with error in rules #1126
Changes from 14 commits
89322bc
5665063
cf72c3d
689e3c9
3109bd6
c68d908
b607ec6
f5e3a0e
5c98306
63a0088
c7f09e0
a53dfc7
d47112b
fd97ee3
a451c02
1c6e8e7
f17c9b1
ae29706
File filter
Filter by extension
Conversations
Jump to
There are no files selected for viewing
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
nit: I'd remove this line (it will be more consistent stylistically).
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@denisvmedia maybe something like this:
return pkg.lint(ruleSet, config, failures)
?There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
We can't simply add a new return argument to the
Apply
function due to the backward compatibility. Someone who usesrevivelib
may depend on this interface.There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Ok @alexandear any idea how to handle errors without of course excuting panic(), we can use in each Apply:
but it is not elegant way :(
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
To maintain backwards compatibility we would have to add backwards compatible wrappers around the newly included changes. And one thing that came to my mind, how do these changes fit the use of
Revive
ingolangci-lint
?There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
ok got it @denisvmedia you want something like that: https://github.com/golangci/golangci-lint/blob/master/pkg/golinters/revive/revive.go#L53 ? but is it possible to do that in separate PR, or it should be done in that PR?