Cirq icon indicating copy to clipboard operation
Cirq copied to clipboard

Create linting plugin that prevents non-module imports in *.py files

Open smadhuk opened this issue 3 years ago • 4 comments

Add a linting plugin to pylint to ensure that only modules are imported

smadhuk avatar Jun 14 '22 23:06 smadhuk

How can I link https://github.com/quantumlib/Cirq/issues/4863 with this PR?

smadhuk avatar Jun 15 '22 22:06 smadhuk

Hm, I understood #4863 a bit differently, with the first priority being to organize and sort imports (3 groups for stdlib, then third party, then cirq imports, with alphabetical sorting in each group). In addition to that I think it's nice to normalize the import style, e.g. internally we use import foo.bar.baz as bas for importing modules and from foo.bar.baz import Qux for importing names from modules. While it's good to discourage the latter, I think it'll be quite difficult to disallow these entirely.

maffoo avatar Jun 16 '22 18:06 maffoo

Ok, I'm going to ask a few questions to figure out the precise scope of what this PR should do.

Is there an automated formatting script (as mentioned in #4863) that automatically formats PRs? Or is it a script that is run while doing local development?

What is the organization of imports going to look like? Will it be 3 groups, where the groups look like (in order):

  1. stdlib imports (alphabetized)
  2. 3rd party imports (alphabetized)
  3. cirq imports (alphabetized)

As for the work done in this PR, should it be removed completely? Or should we generate a warning when a non-module entity is imported?

Thanks in advance!

smadhuk avatar Jun 16 '22 18:06 smadhuk

Is there an automated formatting script (as mentioned in https://github.com/quantumlib/Cirq/issues/4863) that automatically formats PRs? Or is it a script that is run while doing local development?

We have a script check/format-incremental that can be run locally to format files and that also runs as part of ci to ensure that things are formatted before allowing a merge (https://github.com/quantumlib/Cirq/blob/master/.github/workflows/ci.yml#L56). We can add additional step there, such as running isort to cleanup imports.

What is the organization of imports going to look like? Will it be 3 groups, where the groups look like (in order):

  1. stdlib imports (alphabetized)
  2. 3rd party imports (alphabetized)
  3. cirq imports (alphabetized)

Yeah, that looks right. I think this is basically what isort does by default. Internally we use the following configuration in pyproject.toml for isort:

[tool.isort]
profile = 'black'
line_length = 100
order_by_type = false # Sort alphabetically, irrespective of case.
remove_redundant_aliases = true

As for the work done in this PR, should it be removed completely? Or should we generate a warning when a non-module entity is imported?

I'm not sure. I think we should prioritize automatic import sorting, but beyond that I'd be curious what other people think about how strictly we want to enforce things like only importing modules.

maffoo avatar Jun 16 '22 19:06 maffoo