Skip to content

Conversation

@JasonMWhite
Copy link

This PR introduces a new import check, restricting imports to one import per line:

from foo import bar  # OK
from foo import baz # OK
from foo import bar, baz  # forbidden

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

@cfournie
Copy link
Contributor

This isn't strictly mentioned in the Google Python Style Guide; should this instead be included in a separate Shopify-specific checker?

@JasonMWhite
Copy link
Author

'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',
Copy link
Contributor

Choose a reason for hiding this comment

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

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}.

@cfournie
Copy link
Contributor

Sorry, you're right, it is mentioned in the style guide.

'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.',
Copy link
Contributor

Choose a reason for hiding this comment

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

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'

Choose a reason for hiding this comment

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

Statement imports multiple items from '%(module)s'

👍

"resource cleanup activities."),
'C6010': ('Multiple items imported from %(module)s in one import statement.',
'multiple-import-items',
'Separate imports into one item per line.')
Copy link
Contributor

Choose a reason for hiding this comment

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

You could expand upon this to explain the rationale.

CHECKER_CLASS = google_styleguide.GoogleStyleGuideChecker

@contextlib.contextmanager
def assert_adds_code_messages(self, codes, *messages):

Choose a reason for hiding this comment

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

👍

@honkfestival
Copy link

LGTM pending @cfournie's requested changes

@JasonMWhite JasonMWhite merged commit bad55c0 into master Mar 14, 2017
@JasonMWhite JasonMWhite deleted the multiple_import branch March 14, 2017 18:12
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