-
Notifications
You must be signed in to change notification settings - Fork 380
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
[analyzer] Add a new flag to demote missing checker errors to a warning #3866
[analyzer] Add a new flag to demote missing checker errors to a warning #3866
Conversation
21da47a
to
16d8425
Compare
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.
Please see my very minor nitpicks, but LGTM. Thanks for the tests.
action='store_true', | ||
required=False, | ||
default=argparse.SUPPRESS, | ||
help="Emit a warning instead of an error when " |
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.
Maybe include the recommendation from the commit message here too, that it is not advised to use this feature.
LOG.warning(diag_msg) | ||
else: | ||
LOG.error(diag_msg) | ||
LOG.info("If you want to suppress errors relating to unknown " |
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.
Maybe include the recommendation from the commit message here too, that it is not advised to use this feature.
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.
I've considered this, and it usually is my jam to shove my will down on anyones throat, but I figured that any one person that switches an error to a warning must know what they are doing.
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.
Some minor remarks, otherwise looks good.
16d8425
to
6fa0e55
Compare
@@ -1065,7 +1101,7 @@ def test_analyzer_and_checker_config(self): | |||
cwd=self.test_dir, | |||
encoding="utf-8", | |||
errors="ignore") | |||
out, _ = process.communicate() | |||
out, err = process.communicate() |
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.
out, err = process.communicate() | |
out, _ = process.communicate() |
PyLint is complaining about this unused variable, otherwise it looks good.
6fa0e55
to
58e84b7
Compare
Altough discouraged, if someone really insists on having a soft warning when a checker is supplied to --enable or --disable, they can use the new flag '--no-missing-checker-error', which is disabled by default.
58e84b7
to
7ce90c6
Compare
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
…ecker-error 'CodeChecker analyze' was correctly turning errors into warnings when invoked with --no-missing-checker-error, but 'CodeChecker check' wasn't.
…ecker-error 'CodeChecker analyze' was correctly turning errors into warnings when invoked with --no-missing-checker-error, but 'CodeChecker check' wasn't.
…ng_chk Fix a bug in #3866 where check didn't respect --no-missing-checker-error
Altough discouraged, if someone really insists on having a soft warning when a checker is supplied to --enable or --disable, they can use the new flag '--no-missing-checker-error', which is disabled by default.