"Google" profile is not quite Google style

[ssbr]

The Google style support added in #969 has a couple of subtle differences from the actual Google style guide:

1. The import sorting is case sensitive for some reason, although I can't figure out why (case sensitivity is turned off). For example, the following is correct Google style:

   import collections
   import cProfile

   but isort reorders to place the cProfile import before collections:

   $ pipx run --python=python3.8 isort --profile=google test.py --diff
   --- /tmp/test.py:before 2020-09-17 10:28:34.858694
   +++ /tmp/test.py:after  2020-09-17 10:30:01.027299
   @@ -1,2 +1,2 @@
   +import cProfile
    import collections
   -import cProfile

   This is a style error due to the following wording in the Google Python style guide: "Within each grouping, imports should be sorted lexicographically, ignoring case, according to each module’s full package path" (emphasis added)

2. The import sorting sorts by the wrong key. For example, the following is correct Google style:

   from a import z
   from a.b import c

   but isort swaps the order:

   $ pipx run --python=python3.8 isort --profile=google test2.py --diff
   --- /tmp/test2.py:before        2020-09-17 10:34:39.196377
   +++ /tmp/test2.py:after 2020-09-17 10:34:41.925939
   @@ -1,2 +1,2 @@
   +from a.b import c
    from a import z
   -from a.b import c

   This is super subtle -- "a.z" comes after "a.b.c" lexicographically, but the Google style guide, when it refers to the "full package path", actually means the a in from a import z. The style guide did not explicitly say this anywhere, so it's not fair to expect isort to know this. In fact, the only reason I know it is that the internal Google lint rules enforces it. So I had to change the style guide to be more explicit about this, so that I could file this bug report. ;)

   As of google/styleguide@f84020e , the style guide is explicit about this, and now says the following: "Within each grouping, imports should be sorted lexicographically, ignoring case, according to each module’s full package path (the path in from path import ...)."

(Incidentally, if you were curious, the style guide makes no rule about sorting e.g. "foo" with "Foo", and the internal linter accepts either order.)

[hqsz]

First one seems to be simply a omission --case-sensitive option (will be easily solved). I'm not sure if there is an option for the second one. if not, do we need to adding new option? (for example, maybe --group-by-package)

[timothycrosley]

Hi @ssbr,

Thanks for reporting! It's important for us to make the google profile fully compatible with its namesake - I think @lntuition is on the right path on how to accomplish that. If you have any other test example snippets you can provide, or any good public OpenSource projects from Google that utilize the Python style guide - let me know and we can expand the testing on isort's end to include those as well. This will be a priority for the next minor release due out in October.

Thanks!

~Timothy

[ssbr]

@lntuition The thing that confused me w.r.t. --case-sensitive is that the docs claim it is False by default, and the docs for the google profile claim not to override it if I understand correctly. So something is weird there -- maybe just a documentation problem?

@timothycrosley the project in this case was https://github.com/ssbr/refex, but it's definitely not style-complaint already (part of why I wanted to run isort...). I couldn't see any other problems with the isort output though.

I would guess that any Python project in the google org, or a non-google github org that's still run by google or alphabet, would work. Maybe these are some good places to start (picking projects I know have had involvement with the actual Google Python team, and so are more likely to have good style):

• https://github.com/abseil/abseil-py
• https://github.com/google/pytype
• https://github.com/google/yapf

If isort does them dirty even after the two fixes in OP, you can send me the diff and I can try to look into if/why the google style guide differs on that point. (Though chances are good that it's just a style error -- no project is perfect!)

A couple of common things that might crop up are: libraries moving from third-party to first-party section (e.g. enum in py2 vs py3, or typing); ordering problems due to library renames that weren't followed by reordering imports, especially as part of the opensourcing process (this is the big thing that bit me personally).

[timothycrosley]

@ssbr and @lntuition I think what we likely need to turn off per case-sensitivity, is turning off the order by type setting that defaults to True: https://pycqa.github.io/isort/docs/configuration/options/#order-by-type

[ssbr]

Wait, is cProfile interpreted to be a class import? That seems like its own bug.

But yeah, since google style forbids importing anything except modules, grouping by type doesn't make sense, and would only serve to break the ordering rules.

[timothycrosley]

I don't think it's that cProfile is interpreted as a class import, it just looks like order_by_type is overloaded to also mean order modules by case. This obviously is not ideal, especially considering the existence of the case-sensitive option - however, it's pre-existing behaviour that will need to stay the same for the life of the 5.0.0 release cycle to avoid formatting churn. I'm going to tag this issue as one to look back at when we the 6.0.0 release is coming up, as an area where the config options should be improved. NOTE that the improvements to the google profile will move forward independent to this process still slated for the next minor.

[timothycrosley]

@ssbr I've finished including the requested updates to the Google profile in develop, and plan on releasing soon.

If you have a chance, I'd love to get your feedback on the diffs isort produces for the projects you linked. In particular, if isort needs to change anything more to be fully compatible with the Google style for imports:

abseil-py: https://gist.github.com/timothycrosley/56eda241c0d92a25b4b0afba03621ae0
pytype: https://gist.github.com/timothycrosley/ffa0362864b7f14feb3d136c6f3d6567
yapf: https://gist.github.com/timothycrosley/009e2803d03513186dd414349e52b918

Thanks!

~Timothy

[ssbr]

I took a look today :)

tl;dr: I think there's one big type of diff that might matter for other Google projects, where isort is systematically disagreeing with how Google enforces its style guide, but I would personally be happy with the current output, I think.

I'll leave it up to you if you want to reopen for that case or keep closed until someone who wants it chimes in.

Wrong diffs

This is the big one I referenced in the tldr. It isn't "wrong" per se, but is a very large difference in handling to what all Google projects will do, so it might be worth adding a tweak for this.

--- a/absl/flags/_argument_parser.py
+++ b/absl/flags/_argument_parser.py
@@ -27,9 +27,10 @@ import csv
 import io
 import string
 
-from absl.flags import _helpers
 import six
 
+from absl.flags import _helpers

Diffs like this are because absl differs from isort on what it believes "first-party" means -- in the internal source repo that absl is drawn from, anything that is in the third_party directory is considered a third-party package. And since absl is released externally and has third-party contributions, absl goes in third_party. Therefore, when the Google style is applied to absl, it mandates that absl imports (even though they are within absl itself) go in the third-party section. Pathological, right?

Rather than absl being considered first-party, it considers <other-package> (which is not published externally) to be first-party. To make the diff go away, you could add a configuration option that allows users to override what package name is used to decide between first/third party.

TBH I expect some projects would actually prefer isort's current output. I know I would.

---

Not "wrong" per se, but Google allows # pylint directives to go over the line limit, and this reads weird to me as a result.

--- a/pytype/pyi/parser.py
+++ b/pytype/pyi/parser.py
@@ -12,7 +12,8 @@ from pytype.pytd import pytd
 from pytype.pytd import pytd_utils
 from pytype.pytd import slots as cmp_slots
 from pytype.pytd import visitors
-from pytype.pytd.parse import parser_constants  # pylint: disable=g-importing-member
+from pytype.pytd.parse import \
+    parser_constants  # pylint: disable=g-importing-member

LGTM diffs

This is an error in _argument_parser.py, and isort is correct. I can't really explain the order they used, to be honest.

--- a/absl/flags/_argument_parser.pyi
+++ b/absl/flags/_argument_parser.pyi
@@ -15,9 +15,9 @@
 """Contains type annotations for _argument_parser.py."""
 
 
-from typing import Text, TypeVar, Generic, Iterable, Type, List, Optional, Sequence, Any
-
 import enum
+from typing import (Any, Generic, Iterable, List, Optional, Sequence, Text,
+                    Type, TypeVar)

---

Many of the whitespace or ordering changes like this are -- they aren't style errors in Google style, but optional formatting decision. I think isort is justified in making this kind of change.

--- a/absl/flags/_defines.py
+++ b/absl/flags/_defines.py
@@ -33,7 +33,7 @@ from absl.flags import _validators
 
 # pylint: disable=unused-import
 try:
-  from typing import Text, List, Any
+  from typing import Any, List, Text
 except ImportError:
   pass

---

This diff is concerning: the two blank lines are permitted for non-definition things, and required for def/class statements. If isort removed the blank line, and the next line was a def ..., it would have introduced a lint error.

That said, I see a similar diff in pytype that introduces a blank line before a def to change style-nonconformant code to be conforming, so I think it's fine.

--- a/absl/flags/_exceptions.py
+++ b/absl/flags/_exceptions.py
@@ -26,7 +26,6 @@ import sys
 
 from absl.flags import _helpers
 
-
 _helpers.disclaim_module_ids.add(id(sys.modules[__name__]))

"Wrong" but WAI :)

You have no reason to fix this: you can see here they disabled an internal-only lint rule that enforces part of the import style. Naughty devs >:(

(In fact, as they're not importing modules, but classes/functions, that's two style guide violations!)

--- a/setup.py
+++ b/setup.py
@@ -10,7 +10,8 @@ import re
 import shutil
 import sys
 
-from setuptools import setup, Extension  # pylint: disable=g-multiple-import
+from setuptools import Extension  # pylint: disable=g-multiple-import
+from setuptools import setup

[timothycrosley]

Thanks so much for this detailed feedback!

Rather than absl being considered first-party, it considers (which is not published externally) to be first-party. To make the diff go away, you could add a configuration option that allows users to override what package name is used to decide between first/third party.

Based on the information you provided, I think I'm okay with the current behaviour then. isort does provide ways to change how it defines placement - that can be used separately from profiles (which so far have primarily been for the style - though if there was an easy way to include placement rules that applied widely I would do that as well). In this particular case, it sounds like the diff can be resolved simply by adding known_third_party= ['absl'] to a config file.

This diff is concerning: the two blank lines are permitted for non-definition things, and required for def/class statements. If isort removed the blank line, and the next line was a def ..., it would have introduced a lint error.

That's right! By default the start of a method or class will cause isort to use 2 lines, and otherwise use 1. So I think this should be safe.

Thanks!

~Timothy

[timothycrosley]

Update: this has just been released to PyPI in 5.6.0 release of isort: https://pycqa.github.io/isort/CHANGELOG/#560-october-7-2020

Thanks!

~Timothy

[ssbr]

In this particular case, it sounds like the diff can be resolved simply by adding known_third_party= ['absl'] to a config file.

Ah, you're right!

Anyway, I gave it a whirl, and the results are perfect. Thank you!