RobotLocomotion / drake

Model-based design and verification for robotics.
https://drake.mit.edu
Other
3.35k stars 1.27k forks source link

python_lint: `ignore = [402]` causes unrelated errors to spring up? #10537

Closed EricCousineau-TRI closed 5 years ago

EricCousineau-TRI commented 5 years ago

When doing #10536, I first tried just ignoring E402 module level import not at top of file errors that were caused by with statements (see https://github.com/PyCQA/pycodestyle/pull/834).

Example offending patch on 75fa68783:

diff --git a/bindings/pydrake/systems/meshcat_visualizer.py b/bindings/pydrake/systems/meshcat_visualizer.py
index 8594808f1..3f3fe19e2 100644
--- a/bindings/pydrake/systems/meshcat_visualizer.py
+++ b/bindings/pydrake/systems/meshcat_visualizer.py
@@ -21,6 +21,11 @@ from pydrake.systems.framework import (
 )
 from pydrake.systems.rendering import PoseBundle

+with something():
+    import foo
+
+import drake.bar
+

 class MeshcatVisualizer(LeafSystem):
     """

If I run bazel test --config=lint //bindings/pydrake/systems/..., this is the only error triggered.

When I added python_lint_ignore = [402], it triggered E226 missing whitespace around arithmetic operator in a slew of source files.

Patch:

diff --git a/bindings/pydrake/systems/BUILD.bazel b/bindings/pydrake/systems/BUILD.bazel
index 2092e0be3..ce119e19e 100644
--- a/bindings/pydrake/systems/BUILD.bazel
+++ b/bindings/pydrake/systems/BUILD.bazel
@@ -469,4 +469,5 @@
 add_lint_tests(
     cpplint_data = ["//bindings/pydrake:.clang-format"],
     enable_clang_format_lint = True,
+    python_lint_ignore = [402],
 )
jwnimmer-tri commented 5 years ago

The pycodestyle binary has a built-in ignore list (e.g., E121,E123,E126,E226,E24,E704,W503), which if you provide your own list will completely override it.

EricCousineau-TRI commented 5 years ago

Aye. So perhaps a user-friendly fix is to either (a) provide extra_ignore or (b) provide the builtin list as a constant (PYTHON_LINT_IGNORE_DEFAULT or something)?

jwnimmer-tri commented 5 years ago

Doing import drake.bar # noqa is the best resolution, is it not?

EricCousineau-TRI commented 5 years ago

Not really; it's quick-ish, but I don't think it's the best resolution. Also, I'd have to defect the current docs for python_lint(ignore = ...) as this behavior surprised me.

With #10536, I would have had to have added about 10 different #noqas, which seems like a waste if I can just ignore that directive (or fix upstream). (This is low priority since it'd be nice, but not critical, to have a better resolution.)

jwnimmer-tri commented 5 years ago

You are welcome to fix the docs, sure. Personally I think that notating unusual code at every use is a feature, and hiding the suppression in the BUILD file is a bug, but that is irrelevant to whether or not the linter docs are clear.

EricCousineau-TRI commented 5 years ago

SGTM.