Fix `is_module_name()` and improve perf of `is_identifier()` #3795

JonathanPlasse · 2023-03-29T15:11:27Z

For is_module_name()
- Must not be a reserved keyword
- use is_ascii_lowercase() and is_ascii_digit()
Improve perf of is_indentifier()

crates/ruff_python_stdlib/src/identifiers.rs

github-actions · 2023-03-29T16:45:43Z

PR Check Results

Ecosystem

ℹ️ ecosystem check detected changes. (+1, -0, 0 error(s))

zulip (+1, -0)

+ zerver/management/commands/import.py:1:1: N999 Invalid module name: 'import'

Benchmark

Linux

group                                      main                                   pr
-----                                      ----                                   --
linter/all-rules/large/dataset.py          1.02     14.2±0.10ms     2.9 MB/sec    1.00     14.0±0.11ms     2.9 MB/sec
linter/all-rules/numpy/ctypeslib.py        1.01      3.5±0.03ms     4.7 MB/sec    1.00      3.5±0.01ms     4.7 MB/sec
linter/all-rules/numpy/globals.py          1.00    451.8±1.94µs     6.5 MB/sec    1.01    456.9±2.26µs     6.5 MB/sec
linter/all-rules/pydantic/types.py         1.00      6.0±0.06ms     4.3 MB/sec    1.00      6.0±0.05ms     4.2 MB/sec
linter/default-rules/large/dataset.py      1.00      7.2±0.05ms     5.6 MB/sec    1.00      7.3±0.04ms     5.6 MB/sec
linter/default-rules/numpy/ctypeslib.py    1.00   1626.9±5.40µs    10.2 MB/sec    1.01   1637.5±4.37µs    10.2 MB/sec
linter/default-rules/numpy/globals.py      1.00    182.0±0.33µs    16.2 MB/sec    1.01    184.1±2.67µs    16.0 MB/sec
linter/default-rules/pydantic/types.py     1.00      3.3±0.02ms     7.6 MB/sec    1.00      3.4±0.02ms     7.6 MB/sec

Windows

group                                      main                                   pr
-----                                      ----                                   --
linter/all-rules/large/dataset.py          1.00     18.1±0.53ms     2.3 MB/sec    1.00     18.1±0.47ms     2.2 MB/sec
linter/all-rules/numpy/ctypeslib.py        1.00      4.7±0.13ms     3.6 MB/sec    1.02      4.8±0.23ms     3.5 MB/sec
linter/all-rules/numpy/globals.py          1.00   577.4±26.31µs     5.1 MB/sec    1.02   590.6±33.34µs     5.0 MB/sec
linter/all-rules/pydantic/types.py         1.00      7.8±0.24ms     3.3 MB/sec    1.00      7.8±0.23ms     3.3 MB/sec
linter/default-rules/large/dataset.py      1.00      9.5±0.44ms     4.3 MB/sec    1.01      9.5±0.30ms     4.3 MB/sec
linter/default-rules/numpy/ctypeslib.py    1.00      2.1±0.09ms     8.1 MB/sec    1.02      2.1±0.10ms     7.9 MB/sec
linter/default-rules/numpy/globals.py      1.00    244.4±8.81µs    12.1 MB/sec    1.08   263.9±15.92µs    11.2 MB/sec
linter/default-rules/pydantic/types.py     1.00      4.3±0.14ms     6.0 MB/sec    1.06      4.5±0.33ms     5.7 MB/sec

charliermarsh · 2023-03-29T17:20:18Z

Looking at the ecosystem checks, I'd forgotten that I think we shipped a strict version like this in the past and it caused a ton of problems (e.g. with Django migrations), we had to release a hotfix. Let me dig up the issues.

JonathanPlasse · 2023-03-29T18:29:49Z

All the errors except one seem to be migrations. It could easily be ignored with per-file-ignores.
The remaining error is with a file named import.py in zulip. This is not a valid module name and cannot be imported by other modules.
For is_module_name(), the string must be valid ASCII as stated in PEP8.

charliermarsh · 2023-03-29T18:30:47Z

I think I was mixing this up with the fact that we accidentally flagged modules like __main__.py and __version.py__.

MichaReiser · 2023-03-29T19:56:41Z

Woah, nice performance improvement. I'm surprised that it improves by so much

JonathanPlasse · 2023-03-29T20:02:22Z

Indeed, I was not expecting this.

charliermarsh · 2023-03-29T22:24:25Z

My only hangup here is that I'm wondering if we should special-case migration files. If users are always going to end up ignoring them, it'd be nice for us to just "do the right thing" out of the box. At the same time, it's weird to allow what is actually an invalid module name. What do you think?

JonathanPlasse · 2023-03-30T06:04:52Z

How could we avoid false negative if we ignore migration files?

konstin · 2023-03-30T11:35:12Z

Maybe we could start checking if the path contains a component called migrations and whether the filename starts with four digits? This should cover the django projects. alembic seems to use versions/[0-9a-f]{12}(_.*)?.py, but from a quick search i couldn't get the exact semantics

charliermarsh · 2023-03-30T13:14:25Z

I'm temped to say we just ignore anything in a migrations or versions subdirectory.

charliermarsh · 2023-03-30T15:51:39Z

How bout: if migrations or versions is in the path, allow it to start with a number.

- For `is_module_name()` - Must not be a reserved keyword - use is_ascii_lowercase() and is_ascii_digit() - Improve perf of `is_indentifier()`

JonathanPlasse · 2023-03-30T18:24:06Z

Done!

charliermarsh · 2023-03-30T18:25:21Z

Thanks!

charliermarsh · 2023-03-31T18:52:10Z

crates/ruff/src/rules/pep8_naming/rules/invalid_module_name.rs

+            // and other frameworks.
+            if is_migration_file(path) && is_migration_name(&module_name) {
+                return None;
+            }


@JonathanPlasse - What do you think about this? It lets us keep the "project convention" logic out of ruff_python_stdlib (and keeps is_module_name simple since it doesn't rely on directory in any way)... but it does mean we have to check the name twice for valid migrations. However, the most common path is that a module has a valid name anyway, so it may not matter in practice.

It is great. Good separation of concerns.

charliermarsh · 2023-03-31T18:52:33Z

crates/ruff/src/rules/pep8_naming/rules/invalid_module_name.rs

+        .and_then(Path::file_name)
+        .and_then(OsStr::to_str)
+        .map_or(false, |parent| matches!(parent, "versions" | "migrations"))
+}


(E.g., to me, this check is similar to the is_module_file check we already do here -- it feels more at-home in the rule than in the compliance utilities...)

charliermarsh · 2023-03-31T19:01:03Z

crates/ruff/src/rules/pep8_naming/rules/invalid_module_name.rs

+            is_migration_name(&module_name)
+        } else {
+            is_module_name(&module_name)
+        };


@JonathanPlasse - Tweaked it to do the check once like you had before, just separate functions now.

MichaReiser approved these changes Mar 29, 2023

View reviewed changes

crates/ruff_python_stdlib/src/identifiers.rs Outdated Show resolved Hide resolved

crates/ruff_python_stdlib/src/identifiers.rs Outdated Show resolved Hide resolved

charliermarsh reviewed Mar 29, 2023

View reviewed changes

crates/ruff_python_stdlib/src/identifiers.rs Outdated Show resolved Hide resolved

JonathanPlasse force-pushed the fix-is-module-name branch from 092b364 to f8ff0ce Compare March 29, 2023 18:24

JonathanPlasse changed the title ~~Fix is_module_name() and is_identifier()~~ Fix is_module_name() and improve perf of is_identifier() Mar 29, 2023

Fix is_module_name() and improve perf of is_identifier()

389b4db

- For `is_module_name()` - Must not be a reserved keyword - use is_ascii_lowercase() and is_ascii_digit() - Improve perf of `is_indentifier()`

JonathanPlasse force-pushed the fix-is-module-name branch from f8ff0ce to d57dbb0 Compare March 30, 2023 18:22

Ignore migrations and versions folder

c789797

JonathanPlasse force-pushed the fix-is-module-name branch from d57dbb0 to c789797 Compare March 30, 2023 18:23

charliermarsh added 4 commits March 31, 2023 11:09

Merge branch 'main' into fix-is-module-name

764058b

Merge branch 'main' into fix-is-module-name

65ea5ae

Tweak names

ea55bf3

Move migration check into rule

8c814a6

charliermarsh reviewed Mar 31, 2023

View reviewed changes

charliermarsh changed the title ~~Fix is_module_name() and improve perf of is_identifier()~~ Fix is_module_name() and improve perf of is_identifier() Mar 31, 2023

charliermarsh added 2 commits March 31, 2023 15:00

Check once

e80ecaf

Merge branch 'main' into fix-is-module-name

834bf48

charliermarsh reviewed Mar 31, 2023

View reviewed changes

charliermarsh merged commit 968c7df into astral-sh:main Mar 31, 2023

JonathanPlasse deleted the fix-is-module-name branch March 31, 2023 19:28

Tenzer mentioned this pull request Apr 5, 2023

ruff 0.0.261 Homebrew/homebrew-core#127590

Merged

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Fix `is_module_name()` and improve perf of `is_identifier()` #3795

Fix `is_module_name()` and improve perf of `is_identifier()` #3795

JonathanPlasse commented Mar 29, 2023 •

edited

Loading

github-actions bot commented Mar 29, 2023 •

edited

Loading

charliermarsh commented Mar 29, 2023

JonathanPlasse commented Mar 29, 2023

charliermarsh commented Mar 29, 2023

MichaReiser commented Mar 29, 2023 •

edited

Loading

JonathanPlasse commented Mar 29, 2023

charliermarsh commented Mar 29, 2023

JonathanPlasse commented Mar 30, 2023

konstin commented Mar 30, 2023

charliermarsh commented Mar 30, 2023

charliermarsh commented Mar 30, 2023

JonathanPlasse commented Mar 30, 2023

charliermarsh commented Mar 30, 2023

charliermarsh Mar 31, 2023

JonathanPlasse Mar 31, 2023

charliermarsh Mar 31, 2023

charliermarsh Mar 31, 2023

JonathanPlasse Mar 31, 2023

Fix is_module_name() and improve perf of is_identifier() #3795

Fix is_module_name() and improve perf of is_identifier() #3795

Conversation

JonathanPlasse commented Mar 29, 2023 • edited Loading

github-actions bot commented Mar 29, 2023 • edited Loading

PR Check Results

Ecosystem

Benchmark

Linux

Windows

charliermarsh commented Mar 29, 2023

JonathanPlasse commented Mar 29, 2023

charliermarsh commented Mar 29, 2023

MichaReiser commented Mar 29, 2023 • edited Loading

JonathanPlasse commented Mar 29, 2023

charliermarsh commented Mar 29, 2023

JonathanPlasse commented Mar 30, 2023

konstin commented Mar 30, 2023

charliermarsh commented Mar 30, 2023

charliermarsh commented Mar 30, 2023

JonathanPlasse commented Mar 30, 2023

charliermarsh commented Mar 30, 2023

charliermarsh Mar 31, 2023

Choose a reason for hiding this comment

JonathanPlasse Mar 31, 2023

Choose a reason for hiding this comment

charliermarsh Mar 31, 2023

Choose a reason for hiding this comment

charliermarsh Mar 31, 2023

Choose a reason for hiding this comment

JonathanPlasse Mar 31, 2023

Choose a reason for hiding this comment

Fix `is_module_name()` and improve perf of `is_identifier()` #3795

Fix `is_module_name()` and improve perf of `is_identifier()` #3795

JonathanPlasse commented Mar 29, 2023 •

edited

Loading

github-actions bot commented Mar 29, 2023 •

edited

Loading

MichaReiser commented Mar 29, 2023 •

edited

Loading