Add some Google Styleguide formatting rules #4

cfournie · 2017-03-03T00:19:12Z

Let's try to adhere to the Google Python Style Guide using pylint. This PR adds rules for:

"Use imports for packages and modules only";
"Import each module using the full pathname location of the module";
"Avoid global variables.".

cfournie · 2017-03-03T00:20:16Z

.travis.yml

@@ -6,7 +6,6 @@ python:
   - "3.3"
   - "3.4"
   - "3.5"
-   - "3.6"


Sadly pylint doesn't support Python 3.6 yet and our Python 3.6 tests fail.

cfournie · 2017-03-03T00:20:38Z

Makefile

@@ -27,7 +27,7 @@ autolint: autopep8 lint
 run_tests: clean
 	py.test --durations=10 .

-test: autopep8 lint run_tests
+test: autopep8 run_tests lint


Now that we lint the linter with its own rules, we should run tests first.

cfournie · 2017-03-03T00:20:53Z

pylintrc

@@ -19,7 +19,7 @@ persistent=yes

 # List of plugins (as comma separated values of python modules names) to load,
 # usually to register additional checkers.
-load-plugins=
+load-plugins=shopify_python


Let's lint ourselves.

JasonMWhite · 2017-03-03T00:28:15Z

shopify_python/google_styleguide.py

+        self._import_modules_only(node)
+        self._import_full_path_only(node)
+
+    def _import_modules_only(self, node):  # type: (astroid.ImportFrom) -> None


Double-underscore class-private methods as per the code style guidelines.

Should these be private (__) or just protected (_)?

JasonMWhite · 2017-03-03T00:29:08Z

shopify_python/google_styleguide.py

+
+
+class GoogleStyleGuideChecker(checkers.BaseChecker):
+    __implements__ = (interfaces.IAstroidChecker,)


I think we should get in the habit of docstrings for at least these classes.

JasonMWhite · 2017-03-03T00:37:53Z

shopify_python/google_styleguide.py

+                name, _ = name
+                module = '.'.join((node.modname, name))
+                try:
+                    importlib.import_module(module)


This has the potential for side-effects. It would be nice if we could check at least if the rule was disabled globally, and not execute this if required.

cfournie · 2017-03-03T00:47:35Z

shopify_python/google_styleguide.py

+        """
+        def can_import(module):
+            try:
+                importlib.import_module(module)


This has the potential for side-effects. It would be nice if we could check at least if the rule was disabled globally, and not execute this if required.

Can you use inspect for this? Squinting at the docs, it doesn't look like it requires an actual import.

If I could get the path to the source file then I could use inspect. I can get the path by importing the module and then calling __file__on it but that defeats the purpose of this. I'm not sure if there's a way to resolve package.module into path/to/package/module without actually importing the module.

honkfestival

Just a request to avoid importing if possible. And some test cases.

honkfestival · 2017-03-03T15:25:46Z

shopify_python/google_styleguide.py

+    name = 'google-styleguide-checker'
+
+    msgs = {
+        'C9901': ('Imported an object that is not a package or module',


Is there any official advice on choosing the msg_id for new rules? Is e.g. the C99XX range reserved at all, or are we just trying to avoid collisions?

The pylint docs on custom checkers state:

The message id should be a 5-digits number, prefixed with a message category. There are multiple message categories, these being C, W, E, F, R, standing for Convention, Warning, Error, Fatal and Refactoring. The rest of the 5 digits should not conflict with existing checkers and they should be consistent across the checker. For instance, the first two digits should not be different across the checker.

honkfestival · 2017-03-03T18:29:43Z

shopify_python/google_styleguide.py

+        """
+        def can_import(module):
+            try:
+                importlib.import_module(module)


Can you use inspect for this? Squinting at the docs, it doesn't look like it requires an actual import.

honkfestival · 2017-03-03T18:32:52Z

tests/shopify_python/test_google_styleguide.py

+
+    CHECKER_CLASS = google_styleguide.GoogleStyleGuideChecker
+
+    def test_importing_function_fails(self):


Please add a test for importing classes as well.

Oh, and attributes.

honkfestival · 2017-03-03T18:34:25Z

shopify_python/__init__.py

+from pylint import lint
+from shopify_python import google_styleguide
+
+
 __version__ = '0.0.0'


Not really for this PR, but we should have a process for deciding how and when to bump versions.

cfournie · 2017-03-03T19:54:02Z

shopify_python/google_styleguide.py

+    def __import_modules_only(self, node):  # type: (astroid.ImportFrom) -> None
+        """Use imports for packages and modules only."""
+        if hasattr(self.linter, 'config') and 'import-modules-only' in self.linter.config.disable:
+            return  # Skip if disable to avoid side-effects from importing modules


Bails before we call importlib.import_module if this message is disabled.

JasonMWhite · 2017-03-03T20:02:46Z

honkfestival · 2017-03-03T20:09:56Z

shopify_python/google_styleguide.py

+        if hasattr(self.linter, 'config') and 'import-modules-only' in self.linter.config.disable:
+            return  # Skip if disable to avoid side-effects from importing modules
+
+        def can_import(module):


We talked IRL about maybe using importlib's find_spec to perform this check instead.

(there are older, deprecated functions for doing this for versions < 3.4)

honkfestival · 2017-03-03T21:23:06Z

shopify_python/google_styleguide.py

+        Pylint already handles bare-except and broad-except (for Exception).
+        """
+        if node.type.name == 'StandardError':
+            self.add_message('catch-standard-error', node=node)


I think it's fine to catch Exception as long as you re-raise later on in the handler. You might also want to support the exception chaining syntax.

(this is a weak opinion weakly held, btw)

honkfestival · 2017-03-03T21:24:09Z

shopify_python/google_styleguide.py

+
+        Pylint already handles bare-except and broad-except (for Exception).
+        """
+        if node.type.name == 'StandardError':


Don't you mean to check for Exception here as well? (also see below)

Nay, pylint already checks for Exception as broad-except.

sorry, I didn't read carefully enough

honkfestival · 2017-03-03T21:24:58Z

shopify_python/google_styleguide.py

+                  "Use either raise Exception('message') or raise Exception"),
+        'C2606': ('Caught StandardError',
+                  'catch-standard-error',
+                  "Don't catch broad exceptions"),


This line is inconsistent with the specificity of the other two lines above.

honkfestival · 2017-03-03T21:26:12Z

shopify_python/google_styleguide.py

+                  "The larger the except body, the more likely that an exception will be raised"),
+    }
+
+    max_try_exc_finally_body_size = 5


This seems rather low, especially if it's counting raw lines of code instead of logical statements/expressions.

In fact, does it make sense to lint on this at all? I can easily see people disabling this check (or calling out to error handling functions) to avoid it if their error handling is sound.

It's technically counting AST objects, not raw lines of code. E.g. an embedded try/except is one object (even if it has a try body with 5 lines and an except body with another 5).

I'm in favour of this linter rule. Its purpose is to educate and inform people of the expectation, and to catch the accidental violations we all make. If people intentionally rewrite their code to get around linter rules instead of adhering to the principle, that's what code review is for. People can also just disable the check with a comment.

I worry less about what the limit is than that there is a limit. 5 seems reasonable to me. I could be OK with <=10. Another option is to specify an overridable default, similar to the "max lines per module" rule.

For now I'll remove these linter rules and address them in a future PR where I'll probably consider tree size as a measure of complexity over AST children (as it was implemented here).

If people intentionally rewrite their code to get around linter rules instead of adhering to the principle, that's what code review is for.

Code review is also for pointing out complexity problems like the ones this linter rule is meant to address. It's easier to spot too many lines of code in a single try block than too many lines of code split up into functions.

I'm in favour of the spirit of the rule, but not this particular implementation.

Another option is to specify an overridable default, similar to the "max lines per module" rule.

I'd be in favour of something like this. Number of lines might even make the best metric since they're easy to understand (and therefore adjust) by other teams on other projects.

For now I'll remove these linter rules and address them in a future PR

👍

honkfestival · 2017-03-03T21:30:16Z

shopify_python/google_styleguide.py

+                try:
+                    return bool(importlib.util.find_spec(module_name))
+                except AttributeError:
+                    return False


honkfestival · 2017-03-03T21:31:27Z

shopify_python/google_styleguide.py

+            self.add_message('import-full-path', node=node)
+
+    def __avoid_global_variables(self, node):  # type: (astroid.Assign) -> None
+        """Avoid global variables."""


Can you please expand the docstring to explain how this works?

I added a comment below; I assume that implementation details like this should be a comment whereas the purpose of the method should be a docstring.

JasonMWhite · 2017-03-06T15:53:34Z

shopify_python/google_styleguide.py

+
+        Pylint already handles bare-except and broad-except (for Exception).
+        """
+        if node.type.name == 'StandardError':


This fails if no exception type is given, e.g.:

try: foo() except: bar()

node.type is None in this case

JasonMWhite · 2017-03-06T16:15:32Z

shopify_python/google_styleguide.py

+        def can_import(module_name):
+            if sys.version_info >= (3, 4):
+                try:
+                    return bool(importlib.util.find_spec(module_name))


This throws an ImportError when trying to import pyspark, which of course needs some path mangling to import dynamically. I think we should just ignore on ImportError.

honkfestival · 2017-03-06T16:45:30Z

LGTM after addressing @JasonMWhite's pyspark comment

# This is the 1st commit message: added some tets # This is the commit message #2: show me numbers not types # This is the commit message #3: tests and giggles # This is the commit message #4: linted

Add Google Style Guide import-modules-only and import-full-path rules

4963d39

cfournie commented Mar 3, 2017

View reviewed changes

cfournie requested review from JasonMWhite and honkfestival March 3, 2017 00:21

JasonMWhite reviewed Mar 3, 2017

View reviewed changes

cfournie added 2 commits March 2, 2017 19:42

Support importing nonesitent module

011c5ea

Add module doc string

6ff8a46

cfournie commented Mar 3, 2017

View reviewed changes

Add a few more rules and make functions private

40ce171

honkfestival suggested changes Mar 3, 2017

View reviewed changes

cfournie added 4 commits March 3, 2017 14:15

Add more rules and check for import rule disable

dcf85c7

Change message id prefixes

e7a78b0

Remove __dict__ usage

cef8dbf

Add tests for importing a class and module

0c2bff0

cfournie commented Mar 3, 2017

View reviewed changes

JasonMWhite approved these changes Mar 3, 2017

View reviewed changes

honkfestival reviewed Mar 3, 2017

View reviewed changes

cfournie added 3 commits March 3, 2017 15:45

For Python 3 use importlib.utils.find_spec

752a505

Scope find_spec usage to Python 3.4+ and test more Python 3.x versions

a021916

Nope, Python 3.0 to 3.2 are broken

f2969cd

honkfestival reviewed Mar 3, 2017

View reviewed changes

cfournie added 2 commits March 3, 2017 16:40

Be more specific about which broad exception shouldn't be caught

36e002f

Explain how the global var check works

beb2121

JasonMWhite reviewed Mar 6, 2017

View reviewed changes

cfournie added 2 commits March 6, 2017 11:07

Check for whether a catch has an exception

d410ee9

Bump version because we added features

5c9b22b

JasonMWhite reviewed Mar 6, 2017

View reviewed changes

cfournie force-pushed the google_styleguide branch from d6da79b to f84246f Compare March 6, 2017 16:23

Punt the try/except/finally check to a future PR

541710e

cfournie force-pushed the google_styleguide branch from f84246f to 541710e Compare March 6, 2017 16:25

honkfestival approved these changes Mar 6, 2017

View reviewed changes

Handle import error

a81732d

cfournie merged commit 858424f into master Mar 6, 2017

cfournie deleted the google_styleguide branch March 6, 2017 17:32

@@ @@ -6,7 +6,6 @@ python: @@
                  - "3.3"
                  - "3.4"
                  - "3.5"
-                 - "3.6"



		class GoogleStyleGuideChecker(checkers.BaseChecker):
		__implements__ = (interfaces.IAstroidChecker,)


		CHECKER_CLASS = google_styleguide.GoogleStyleGuideChecker

		def test_importing_function_fails(self):

Add some Google Styleguide formatting rules #4

Add some Google Styleguide formatting rules #4

Conversation

cfournie commented Mar 3, 2017 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

honkfestival left a comment • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

JasonMWhite commented Mar 3, 2017

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

cfournie Mar 3, 2017 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

JasonMWhite Mar 6, 2017 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

honkfestival commented Mar 6, 2017

cfournie commented Mar 3, 2017 •

edited

Loading

honkfestival left a comment •

edited

Loading

cfournie Mar 3, 2017 •

edited

Loading

JasonMWhite Mar 6, 2017 •

edited

Loading