Skip to content

Conversation

@cfournie
Copy link
Contributor

@cfournie cfournie commented Mar 13, 2017

This PR adds a check to ensure that source code disables rules by message name and not message code. Fixes #32.

If a bad message code is mentioned in a disable comment, in addition to bad-option-value we output a message suggesting unknown as a code (in case bad-option-value has been disabled):

shopify_python/shopify_styleguide.py:3:0 E0012(bad-option-value) Bad option value 'X9999'
shopify_python/shopify_styleguide.py:3:0 C6101(disable-name-only) X9999 disabled as a message code, use 'unknown' instead

@erikwright
Copy link
Contributor

I believe the checkers have access to the linter as a member of self, and that the linter provides a means to look up a symbolic check name by id. Can you investigate doing that and outputting the correct value in the error message?

""".strip())

with self.assertAddsMessages(*[
pylint.testutils.Message('disable-name-only', line=line, args={'code': code, 'name': 'unknown'})

Choose a reason for hiding this comment

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

Why doesn't the linter know the suggested name for any of these codes?

Copy link
Contributor Author

@cfournie cfournie Mar 13, 2017

Choose a reason for hiding this comment

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

In tests it's not a real linter, it's a pylint.testutils.UnittestLinter which has no msgs_store to query.

Choose a reason for hiding this comment

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

👍

Choose a reason for hiding this comment

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

If there's a way to add this info to the linter to prove that it actually pulls it out, then we should add that. If it's too hacky though, don't worry about it.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Done

@cfournie cfournie merged commit c913a55 into master Mar 14, 2017
@cfournie cfournie deleted the disable_by_name branch March 14, 2017 18:04
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

3 participants