Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

lint: Should add basic linter for Python import statements #13571

Open
3 tasks
EricCousineau-TRI opened this issue Jun 18, 2020 · 6 comments
Open
3 tasks

lint: Should add basic linter for Python import statements #13571

EricCousineau-TRI opened this issue Jun 18, 2020 · 6 comments
Assignees

Comments

@EricCousineau-TRI
Copy link
Contributor

EricCousineau-TRI commented Jun 18, 2020

Should formalize import statements
Relates #13572 (issue) in that this should be a idempotent projection to one format (ideal thing for linting)

Steps:

  • Styleguide PR
  • Lint implementation + unittests
  • Application to codebase

Some design thoughts:

  • From PEP008, should delineate builtin, third, and first-part sources
  • Should be extensible for Anzu
  • Should handle ensuring pydrake is imported before torch (for Anzu)
  • Should also handle mut and mut_* modules being imported first (or encourage it, as well)

My preferred styles and ordering (aside from the PEP8 sections):

# Basic import
import module
# Single symbol import
from module import SingleSymbol
# Module import from package
from package_1.sub_package import module
# import package_1.sub_package.module as module  # Nah. I think this is silly, trying too hard for line-lexical sorting.
# Abbreviated alias
import package_2.sub_package.module as m2
# Super line wrap
from package_3.sub_package.sub_sub_package.sub_sub_sub_package import (  # noqa
  LineLengthExceeded,
)
# Symbol imports
# - No linewrap needed
from yyy_module import YyySymbol1, YyySymbol2
# - Linewrap needed
from zzz_module import (
   ZzzSymbolA,
   ZzzSymbolB,
   ZzzSymbolC,
   ZzzSymbolD,
)

EDIT: On second thought, perhaps import pkg.sub as sub is better than from pkg import sub, just as a way to show that it's a module, not a symbol.

\cc @thduynguyen @RussTedrake

@EricCousineau-TRI EricCousineau-TRI self-assigned this Jun 18, 2020
@EricCousineau-TRI EricCousineau-TRI changed the title style: Should add basic linter for Python import statements lint: Should add basic linter for Python import statements Jun 18, 2020
@jwnimmer-tri
Copy link
Collaborator

jwnimmer-tri commented Jun 18, 2020

Using a community tool would be best, if there is a suitable one. First search shows https://github.com/timothycrosley/isort as a contender.

(Found via mention at psf/black#333. To the extent we canonicalize on formatting, IMO we should prefer psf/black as the arbiter.)

@jwnimmer-tri
Copy link
Collaborator

If I'm reading the intention here correctly, then FYI in Anzu we have an implementation of this already. We could post that code somewhere for someone to adapt it to Drake, if there was interest.

@svenevs
Copy link
Contributor

svenevs commented Jan 24, 2024

xref as likely related via isort #19827

@jwnimmer-tri
Copy link
Collaborator

Here's some pyproject.toml settings for isort that might be relevant:

[tool.isort]
# For more information:
# - https://timothycrosley.github.io/isort/
# - (Permalink) https://github.com/timothycrosley/isort/blob/5.1.3/README.md

# Based on `profile = black`, but with customizations.
force_grid_wrap = false
force_sort_within_sections = true
honor_noqa = true
include_trailing_comma = true
multi_line_output = 3  # Vertical Hanging Indent
use_parentheses = true
# We turn this off because we want symbols like LCM to be simple sorted.
order_by_type = false
case_sensitive = true

# [ Sections ]
# See: https://github.com/timothycrosley/isort#custom-sections-and-ordering
# This determines the ordering of imports.
sections = [
    # {builtin}
    "FUTURE",
    # {builtin} + extra_standard_library
    "STDLIB",
    # known_third_party
    "THIRDPARTY",
    # known_first_party
    "FIRSTPARTY",
    # {builtin}
    "LOCALFOLDER",
]
default_section = "THIRDPARTY"

known_first_party = [
    "drake",
    "pydrake",
]

@jwnimmer-tri
Copy link
Collaborator

Also a reminder from f2f: we don't need a big-bang PR that adds linters and fixes everything. Instead, we should PR the tool and one 1 sample subdir that enables it. Then, incrementally we'll work on getting more and more parts of the tree to pass.

@jwnimmer-tri
Copy link
Collaborator

The import order linter & auto-fixer imagined here will prescribe exactly how the imports should look.

In terms of choosing a configuration file for that tool, the first-order goal should be to match (as closely as reasonable) the existing import ordering that Drake's code already uses. Of course some existing code will be wrong/inconsistent and the linter will end up fixing those files, but the first stab at config file settings should be ones that have as small of diffs as practical.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Development

No branches or pull requests

4 participants