Implement flake8-i18n #3741

leiserfg · 2023-03-26T15:03:29Z

Add skelleton code
Add fixtures for flake8_i18n
Add tests
Add implementation and insta snapshots
Add license entry

Closes #3294.

github-actions · 2023-03-26T15:19:00Z

PR Check Results

Ecosystem

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

zulip (+1, -0)

+ corporate/lib/stripe.py:1010:17: INT001 f-string is resolved before function call; consider `_("string %s") % arg`

Benchmark

Linux

group                                      main                                   pr
-----                                      ----                                   --
linter/all-rules/large/dataset.py          1.00     14.5±0.08ms     2.8 MB/sec    1.00     14.5±0.06ms     2.8 MB/sec
linter/all-rules/numpy/ctypeslib.py        1.00      3.8±0.03ms     4.4 MB/sec    1.00      3.8±0.02ms     4.4 MB/sec
linter/all-rules/numpy/globals.py          1.01    435.3±1.56µs     6.8 MB/sec    1.00    433.0±1.58µs     6.8 MB/sec
linter/all-rules/pydantic/types.py         1.00      6.4±0.02ms     4.0 MB/sec    1.02      6.5±0.05ms     3.9 MB/sec
linter/default-rules/large/dataset.py      1.01      7.9±0.06ms     5.2 MB/sec    1.00      7.8±0.04ms     5.2 MB/sec
linter/default-rules/numpy/ctypeslib.py    1.01   1712.6±9.61µs     9.7 MB/sec    1.00   1694.7±2.78µs     9.8 MB/sec
linter/default-rules/numpy/globals.py      1.00    175.8±0.89µs    16.8 MB/sec    1.01    177.0±0.44µs    16.7 MB/sec
linter/default-rules/pydantic/types.py     1.01      3.7±0.01ms     6.9 MB/sec    1.00      3.6±0.01ms     7.0 MB/sec

Windows

group                                      main                                   pr
-----                                      ----                                   --
linter/all-rules/large/dataset.py          1.00     15.5±0.13ms     2.6 MB/sec    1.00     15.6±0.15ms     2.6 MB/sec
linter/all-rules/numpy/ctypeslib.py        1.00      4.2±0.06ms     4.0 MB/sec    1.00      4.2±0.05ms     4.0 MB/sec
linter/all-rules/numpy/globals.py          1.01    463.1±7.69µs     6.4 MB/sec    1.00    458.3±8.02µs     6.4 MB/sec
linter/all-rules/pydantic/types.py         1.00      6.9±0.08ms     3.7 MB/sec    1.00      6.9±0.10ms     3.7 MB/sec
linter/default-rules/large/dataset.py      1.00      8.3±0.06ms     4.9 MB/sec    1.00      8.3±0.06ms     4.9 MB/sec
linter/default-rules/numpy/ctypeslib.py    1.00  1785.6±17.06µs     9.3 MB/sec    1.01  1794.8±17.34µs     9.3 MB/sec
linter/default-rules/numpy/globals.py      1.00    185.5±2.06µs    15.9 MB/sec    1.00    185.5±6.65µs    15.9 MB/sec
linter/default-rules/pydantic/types.py     1.01      3.9±0.03ms     6.6 MB/sec    1.00      3.9±0.03ms     6.6 MB/sec

MichaReiser · 2023-03-27T07:16:32Z

crates/ruff/src/checkers/ast/mod.rs

+                        if let Some(diagnostic) =
+                            flake8_i18n::rules::f_string_in_i18n_func_call(args)
+                        {
+                            self.diagnostics.push(diagnostic);
+                        }


Nit:

Suggested change

if let Some(diagnostic) =

flake8_i18n::rules::f_string_in_i18n_func_call(args)

{

self.diagnostics.push(diagnostic);

}

self.diagnostics.extend(flake8_i18n::rules::f_string_in_i18n_func_call(args))

MichaReiser · 2023-03-27T07:19:36Z

crates/ruff/src/rules/flake8_i18n/settings.rs

+    pub function_names: Option<Vec<String>>,
+
+    #[option(
+        default = r#"["_", "gettext", "ngettext"]"#,
+        value_type = "list[str]",
+        example = r#"
+            [tool.ruff.flake8-i18n]
+            extend-function-names = ["ugettetxt"]
+        "#
+    )]
+    /// This will be appended to the function_names
+    /// (or to the default values if that field is missing)
+    pub extend_function_names: Option<Vec<String>>,


Can we use Vec<String> directly or is it important to know whether the settings where empty vs None

True, it's fine to use a regular Vec here.

After this change tests are failing in CI but no idea why, 'cause they pass locally.

I see, some changes in main, I will rebase to fix them.

crates/ruff/resources/test/fixtures/flake8_i18n/INT001.py

charliermarsh · 2023-03-27T17:45:56Z

Nice work here. I made a few minor but otherwise merging as-is. Thanks for getting involved :)

charliermarsh · 2023-03-27T17:49:15Z

crates/ruff/src/rules/flake8_i18n/settings.rs

+                .unwrap_or_else(default_func_names)
+                .into_iter()
+                .chain(options.extend_function_names)
+                .collect(),


I changed this to use an iterator.

AA-Turner · 2023-03-27T20:37:02Z

LICENSE

@@ -195,6 +195,8 @@ are:
    CONNECTION WITH THE SOFTWARE OR THE USE OR OTHER DEALINGS IN THE SOFTWARE.
  """

+- flake8-i18n, licensed as "GNU General Public License v2 (GPLv2)".


@charliermarsh -- I'm not certain on this but wanted to ask the question -- does including a GPL tool in Ruff mean that (all/part of) Ruff would be considered a derivative work for the purposes of GPL, and therefore that Ruff would need to be re-licenced (or this PR reverted or etc)? This is the first GPL tool to be integrated, and I wanted to flag the concern as early as possible. Thanks, Adam

Lemme look into it.

There are already other parts including GPL3 (flake8-django for instance).

edit: well only that one, but I think GPL3 is even more infectious than GPL2.

I'll have to get some more informed opinions on this, I'm not sure myself on whether these constitute derivative works.

If they do, and we'd have to re-license under GPL, then I'd err on the side of removing them, or going back to the authors and asking if they'd consider re-licensing under MIT or a dual license.

flake8-django is confusing because on PyPI it appears dual-licensed, and it includes the MIT trove classifiers:

I'll have to get some more informed opinions on this, I'm not sure myself on whether these constitute derivative works.

Sorry to cause the trouble and thank you both for looking in to this.

flake8-django is confusing because on PyPI it appears dual-licensed, and it includes the MIT trove classifiers:

It seems this has been noted already: rocioar/flake8-django#101 and rocioar/flake8-django#102. Sorry not to have flagged this at the time for flake8-django, but it seems the MIT trove classifier may have been a mistake when that project adopted Poetry.

A

No, thank you for calling this out! It's important to get right. I'll try to reach out to the flake8-django maintainer.

Hum... This is such a common "idea", I've done it before without even knowing about flake8-i18n. We were working in a project and switched to Python 3.6 and many members of a the team started to use f-strings inside _. I became such a common mistake we had to "invent" our checker.

I'm not a lawyer, but I don't think this can be gathered as derivative work.

This reverts commit 5cb1203.

leiserfg · 2023-03-27T21:04:38Z

@charliermarsh should I talk to @vharitonsky to see if he can change the license for us or will you do it?

charliermarsh · 2023-03-27T21:05:11Z

@leiserfg - If you're willing to that'd be great.

leiserfg · 2023-03-28T07:01:40Z

To be honest, I don't think this can be considered derivative work as we barely used some of their ideas, and ideas can be patented but not copyrighted.

leiserfg · 2023-03-28T19:24:42Z

@charliermarsh I found another implementation that does basically the same and is licensed under BSD, is it fine if we reference that one instead?

charliermarsh · 2023-03-28T19:58:48Z

@leiserfg - Yeah that looks fine to me. I do think we could in theory include these rules without referencing flake8-i18n, especially since the issue that was raised on Ruff pre-dated any knowledge of that plugin. But it seems most straightforward to base it on flake8-gettext. Want to put up a PR?

charliermarsh · 2023-03-28T19:59:03Z

Let's continue to use the INT codes (which I think differs from either of those plugins).

leiserfg · 2023-03-28T21:06:58Z

#3785

charliermarsh · 2023-03-28T21:44:20Z

Thx!

leiserfg force-pushed the implement-flake8-i18n branch from ed6f44a to 58e2197 Compare March 26, 2023 15:07

MichaReiser reviewed Mar 27, 2023

View reviewed changes

leiserfg force-pushed the implement-flake8-i18n branch from 4477b04 to fbc35f2 Compare March 27, 2023 08:05

leiserfg added 9 commits March 27, 2023 10:47

Add skelleton code

469faf7

Add fixtures for flake8_i18n

7d4108f

Add tests

a587553

Add implementation and insta snapshots

1861ed3

Add license entry

e77dcf5

Apply fmt fixes

b19486c

Use extend instead of checking optional

bbbe078

Use vec for extra args as empty vec works fine there

f811ae7

Set default value for extend_function_names and update schema

41b4ba1

mvaled reviewed Mar 27, 2023

View reviewed changes

crates/ruff/resources/test/fixtures/flake8_i18n/INT001.py Show resolved Hide resolved

Update insta snapshots after rebasing master

e8a8483

leiserfg force-pushed the implement-flake8-i18n branch from f3ed6bb to e8a8483 Compare March 27, 2023 08:54

charliermarsh added 2 commits March 27, 2023 13:39

Modify signatures; add to docs

0fa4e41

Adjust defaults

411a0d6

charliermarsh added the rule Implementing or modifying a lint rule label Mar 27, 2023

Edit license

99f4970

charliermarsh force-pushed the implement-flake8-i18n branch from 4a37e88 to 99f4970 Compare March 27, 2023 17:48

charliermarsh reviewed Mar 27, 2023

View reviewed changes

charliermarsh enabled auto-merge (squash) March 27, 2023 17:49

charliermarsh merged commit 5cb1203 into astral-sh:main Mar 27, 2023

AA-Turner reviewed Mar 27, 2023

View reviewed changes

charliermarsh added a commit that referenced this pull request Mar 27, 2023

Revert "Implement flake8-i18n (#3741)"

dc13611

This reverts commit 5cb1203.

charliermarsh mentioned this pull request Mar 27, 2023

Revert "Implement flake8-i18n (#3741)" #3765

Merged

leiserfg mentioned this pull request Mar 27, 2023

Re-licencing for usage in ruff vharitonsky/flake8-i18n#2

Closed

charliermarsh added a commit that referenced this pull request Mar 27, 2023

Revert "Implement flake8-i18n (#3741)" (#3765)

c3917ea

chenrui333 mentioned this pull request Mar 30, 2023

ruff 0.0.260 Homebrew/homebrew-core#127014

Merged

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Implement flake8-i18n #3741

Implement flake8-i18n #3741

leiserfg commented Mar 26, 2023 •

edited by charliermarsh

Loading

github-actions bot commented Mar 26, 2023 •

edited

Loading

MichaReiser Mar 27, 2023

MichaReiser Mar 27, 2023

leiserfg Mar 27, 2023

leiserfg Mar 27, 2023

leiserfg Mar 27, 2023

charliermarsh commented Mar 27, 2023

charliermarsh Mar 27, 2023

AA-Turner Mar 27, 2023 •

edited

Loading

charliermarsh Mar 27, 2023

leiserfg Mar 27, 2023 •

edited

Loading

charliermarsh Mar 27, 2023

AA-Turner Mar 27, 2023

charliermarsh Mar 27, 2023

mvaled Mar 28, 2023

leiserfg commented Mar 27, 2023

charliermarsh commented Mar 27, 2023

leiserfg commented Mar 28, 2023

leiserfg commented Mar 28, 2023

charliermarsh commented Mar 28, 2023

charliermarsh commented Mar 28, 2023

leiserfg commented Mar 28, 2023

charliermarsh commented Mar 28, 2023

Implement flake8-i18n #3741

Implement flake8-i18n #3741

Conversation

leiserfg commented Mar 26, 2023 • edited by charliermarsh Loading

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

PR Check Results

Ecosystem

Benchmark

Linux

Windows

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

charliermarsh commented Mar 27, 2023

Choose a reason for hiding this comment

AA-Turner Mar 27, 2023 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

leiserfg Mar 27, 2023 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

leiserfg commented Mar 27, 2023

charliermarsh commented Mar 27, 2023

leiserfg commented Mar 28, 2023

leiserfg commented Mar 28, 2023

charliermarsh commented Mar 28, 2023

charliermarsh commented Mar 28, 2023

leiserfg commented Mar 28, 2023

charliermarsh commented Mar 28, 2023

leiserfg commented Mar 26, 2023 •

edited by charliermarsh

Loading

github-actions bot commented Mar 26, 2023 •

edited

Loading

AA-Turner Mar 27, 2023 •

edited

Loading

leiserfg Mar 27, 2023 •

edited

Loading