-
Notifications
You must be signed in to change notification settings - Fork 2k
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
Fix-up (gometalinter) linting config #2163
Conversation
ping @silvin-lubecki @kolyshkin @tiborvass @vdemeester PTAL |
The configuration abused "Exclude" to exclude file-paths by filtering on the output, however, the `Skip` option was designed for that, whereas `Exclude` is for matching warnings. An explicit "Skip" was added for "vendor", because even though the vendor directory should already be ignored by the linter, in some situations, it still seemed to warn on issues, so let's explicitly ignore it. Signed-off-by: Sebastiaan van Stijn <[email protected]>
c8f9b12
to
71e525f
Compare
removed one commit, as it may only be applicable with other changes |
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.
LGTM
side note: why don't we switch to |
], | ||
"Exclude": [ | ||
"parameter .* always receives", | ||
"_esc(Dir|FS|FSString|FSMustString) is unused" |
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.
the commit message does not explain where this line is coming from
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.
Ahm, yes. Those are from the cli/compose/schema/bindata.go
file. Looks like some linters don't get passed the Skip
configuration correctly, and still invalidate.
We really need to get rid of gometalinter in this repository; there's a WIP pull request #1797, but I need to find some time to carry it.
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.
LGTM
@kolyshkin I have a PR somewhere (#1797 ) to switch to |
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.
LGTM
gometalinter: fix configuration
The configuration abused "Exclude" to exclude file-paths by filtering on the output, however, the
Skip
option was designed for that, whereasExclude
is for matching warnings.An explicit "Skip" was added for "vendor", because even though the vendor directory should already be ignored by the linter, in some situations, it still seemed to warn on issues, so let's explicitly ignore it.