Conversation
|
This isn't strictly mentioned in the Google Python Style Guide; should this instead be included in a separate Shopify-specific checker? |
shopify_python/google_styleguide.py
Outdated
| 'finally-too-long', | ||
| "The larger the 'finally' body size, the more likely that an exception will be raised during " | ||
| "resource cleanup activities."), | ||
| 'C2610': ('%s imports multiple items in one import statement', |
There was a problem hiding this comment.
Specify args in the message if you want formatted output; currently it shows:
C: 8, 0: %s imports multiple items in one import statement (multiple-import-items)
Perhaps reword it to read:
Multiple items imported from %(module)s in one import statement.
And in add_message provide args={'module': node.modname}.
|
Sorry, you're right, it is mentioned in the style guide. |
21bff1b to
464a216
Compare
shopify_python/google_styleguide.py
Outdated
| 'finally-too-long', | ||
| "The larger the 'finally' body size, the more likely that an exception will be raised during " | ||
| "resource cleanup activities."), | ||
| 'C6010': ('Multiple items imported from %(module)s in one import statement.', |
There was a problem hiding this comment.
These don't commonly end in a period and are usually shorter, e.g.:
shopify_python/google_styleguide.py:192:0 I0011(locally-disabled) Locally disabling no-member (E1101)
If printed this would look like:
shopify_python/google_styleguide.py:192:0 C6010(multiple-import-items) Multiple items imported from os in one import statement. (C6010)
Maybe rephrase as:
Statement imports multiple items from '%(module)s'
or
Multiple items imported from '%(module)s'
There was a problem hiding this comment.
Statement imports multiple items from '%(module)s'
👍
shopify_python/google_styleguide.py
Outdated
| "resource cleanup activities."), | ||
| 'C6010': ('Multiple items imported from %(module)s in one import statement.', | ||
| 'multiple-import-items', | ||
| 'Separate imports into one item per line.') |
There was a problem hiding this comment.
You could expand upon this to explain the rationale.
| CHECKER_CLASS = google_styleguide.GoogleStyleGuideChecker | ||
|
|
||
| @contextlib.contextmanager | ||
| def assert_adds_code_messages(self, codes, *messages): |
|
LGTM pending @cfournie's requested changes |
464a216 to
25c72b2
Compare
This PR introduces a new import check, restricting imports to one import per line:
This PR also ran up against other rule checks, so I created a new assertion checker that allows you to limit which codes are verified. This makes tests less fragile (no interdependencies between codes). Also, consumers may disable individual codes, so it's important that we test a code in isolation, whether or not it also fails other checks.
@cfournie @honkfestival