Use ruff format #12478

sbidoul · 2024-01-15T13:08:07Z

Coming from #12454 (comment) I made a quick try to use ruff-format instead of black.

pfmoore

Some nitpicks, some cases where I dislike ruff's choices, and a couple of flat-out issues that need addressing (probably by ruff). Specifically, the failure to respect # fmt: off, and the way #type: ignore comments are changed to affect different code, are showstoppers IMO.

Also, it feels like ruff's line length setting is being applied differently than black's (or the settings themselves differ). It's not a disaster, but a couple of the wrapping points seem to have changed for no obvious reason.

pfmoore · 2024-01-15T14:06:26Z

src/pip/_internal/commands/install.py

-                os.path.exists(options.target_dir) and
-                not os.path.isdir(options.target_dir)
+                os.path.exists(options.target_dir)
+                and not os.path.isdir(options.target_dir)


This bothers me because it's in a # fmt: off block. Does ruff not support that directive?

Ruff is more limited in where it supports supression statements: astral-sh/ruff#6338

I suspect this is a location where it does not support them, ruff has been considering adding a warning for this case: astral-sh/ruff#6611

pfmoore · 2024-01-15T14:07:49Z

src/pip/_internal/metadata/_json.py

@@ -64,7 +64,8 @@ def sanitise_header(h: Union[Header, str]) -> str:
        key = json_name(field)
        if multi:
            value: Union[str, List[str]] = [
-                sanitise_header(v) for v in msg.get_all(field)  # type: ignore
+                sanitise_header(v)
+                for v in msg.get_all(field)  # type: ignore


This may be a semantic change. If the # type: ignore directive is per-line (rather than per-expression) then the sanitise_header(v) will now be type-checked.

mypy is happy with it, so it should be fine

Seems this is an implementation based semantic (i.e. no standard), for MyPy you should be getting a consistent behavior on Python 3.8+ (but potentially not if you were to run it on 3.7, which Pip's CI doesn't do): python/mypy#6648

You may have misunderstood my comment (or I may have misunderstood your reply). From PEP 484, "The # type: ignore comment should be put on the line that the error refers to". This suggests that if there were an error in line 67 (sanitise_header(v)), the ignore comment wouldn't apply to it now, because that ignore is on line 68.

I know I'm being very pedantic here, but I'm bothered by the general idea that ruff is failing to reformat in a way that's semantically identical.

The case in test_utils.py below is even more problematic, IMO.

Seems this is an implementation based semantic (i.e. no standard)

Well, PEP 484 states the behaviour. But I agree that there's a lot of typing behaviour that's highly dependent on the type checker you use. I have my issues with that, but they aren't relevant here. What is relevant is that ruff shouldn't be making changes it can't be 100% certain are semantically neutral.

Please understand, I'm not complaining about ruff here - I'm a huge fan of it. But the formatting capability is relatively new, and I'm not sure I want pip to be an early adopter if there are still issues like this that need to be discussed and resolved (potentially between the formatter and type checker communities - expression-based type ignores may well be a far better resolution, so ruff may be making the right call here).

pfmoore · 2024-01-15T14:09:07Z

tests/functional/test_completion.py

@@ -176,7 +176,7 @@ def test_completion_alone(autocomplete_script: PipTestEnvironment) -> None:
    assert (
        "ERROR: You must pass --bash or --fish or --powershell or --zsh"
        in result.stderr
-    ), ("completion alone failed -- " + result.stderr)
+    ), "completion alone failed -- " + result.stderr


Hmm, I prefer black's approach of leaving the parentheses alone here. But I guess the whole point of using an automated formatter is that I'm supposed to not have opinions like that 🙄

pfmoore · 2024-01-15T14:09:46Z

tests/functional/test_completion.py

-        "autocomplete function completed <file> or <dir> that "
-        "should not be completed"
-    )
+    ), "autocomplete function completed <file> or <dir> that should not be completed"


... and even more so here. I much prefer the split and parenthesised version. But whatever, I guess.

pfmoore · 2024-01-15T14:11:02Z

tests/unit/test_configuration.py

+            == (
+                # Use new config file
+                get_configuration_files()[kinds.USER][1]
+            )


This is definitely worse than the black choice.

pfmoore · 2024-01-15T14:12:08Z

tests/unit/test_utils.py

@@ -249,8 +249,7 @@ def test_rmtree_errorhandler_reraises_error(tmpdir: Path) -> None:
    # Create directory without read permission
    subdir_path = tmpdir / "subdir"
    subdir_path.mkdir()
-    path = str(subdir_path)
-    os.chmod(path, stat.S_IWRITE)
+    os.chmod(subdir_path, stat.S_IWRITE)


Did ruff do this, or was it a manual change? I'm very concerned if this was a ruff change. If it was manual, it's unrelated to this PR, isn't it?

Manual change indeed. I did it in a separate commit. I detected it because ruff split it in 3 lines which revealed that the # type: ignore was addressing two separate typing issues. So thanks ruff in this case.

pfmoore · 2024-01-15T14:12:41Z

tests/unit/test_utils.py

-                mock_func, path, sys.exc_info()  # type: ignore[arg-type]
+                mock_func,
+                subdir_path,
+                sys.exc_info(),  # type: ignore[arg-type]


Again, I'm not convinced that this doesn't change what's affected by the # type: ignore directive.

IMO, the correct reformatting here has to be

mock_func, # type: ignore[arg-type] subdir_path, # type: ignore[arg-type] sys.exc_info(), # type: ignore[arg-type]

This seems to me like a straight bug in ruff.

There appears to be a related discussion on ruff side here: astral-sh/ruff#8286 (comment)

But no specific issue open on ruff side that I can find which exactly matches your concern.

I added a note about this case to that issue.

pfmoore · 2024-01-15T14:14:06Z

tools/release/__init__.py

-        silent=True,
-    )
+    cmd = ["git", "tag", "-m", message, tag_name]
+    session.run(*cmd, external=True, silent=True)


Manual change, unrelated to ruff? I can accept that the changed version is better, but IMO we shouldn't be mixing changes to the automation and manual reformatting in the one PR.

Yes I should have done that in another commit.

pfmoore · 2024-01-15T14:15:25Z

tools/release/__init__.py

            "--",
-            ".", str(git_checkout_dir),
+            ".",
+            str(git_checkout_dir),
            # fmt: on


Looks like ruff is flat-out ignoring #fmt: off here. That's a ruff bug (or at best, an annoying decision to not respect the standard convention) and should be reported to ruff. IMO this is a blocker to us adopting ruff.

I think this is Ruff needing the comments at statement level.

See https://docs.astral.sh/ruff/formatter/#format-suppression which says instead of:

[ # fmt: off '1', # fmt: on '2', ]

Do:

# fmt: off [ '1', '2', ] # fmt: on

Understood. I still think it's a problem with ruff, though. At the absolute minimum it should warn if it's ignoring formatting directives. (Specifically if it's ignoring directives that other formatters like black would respect).

They have an open issue on warning, I'm sure sure supportive feedback would be welcome: astral-sh/ruff#6611

pfmoore · 2024-01-15T14:18:29Z

Thanks for doing this - you beat me to it 🙂

notatallshaw · 2024-01-15T14:26:50Z

I don't think linting rule ISC001 should be turned off, it's a valuable rule that has caught real bugs in code bases I work with.

I understand that it may require a PR author a manually fix to the code to get a successful pass of both the ruff linter and the ruff formerter with rule ISC001, and therefore ruff complains about it, but that's already the case with running the ruff linter and running black, just black isn't aware of this linting rule.

FYI the discussion on ruff side is here: astral-sh/ruff#8272

sbidoul · 2024-01-15T14:30:05Z

I don't think linting rule ISC001 should be turned off, it's a valuable rule that has caught real bugs in code bases I work with.

It's ruff that recommended to disable it so I complied :).

notatallshaw · 2024-01-15T14:36:47Z

It's ruff that recommended to disable it so I complied :).

Yes, my comment explains why ruff warns about it and the reasoning I don't think it should be turned off:

I understand that it may require a PR author a manually fix to the code to get a successful pass of both the ruff linter and the ruff formerter with rule ISC001, and therefore ruff complains about it, but that's already the case with running the ruff linter and running black, just black isn't aware of this linting rule.

And ruff is considering turning the warning off, see: astral-sh/ruff#8272

sbidoul added 2 commits January 15, 2024 13:37

Use ruff-format

2530ce8

Improve typing in test_utils

f41c836

pfmoore requested changes Jan 15, 2024

View reviewed changes

Fogapod mentioned this pull request Jan 17, 2024

replace black with ruff format unitystation/central-command#76

Merged

notatallshaw mentioned this pull request Mar 24, 2024

Update black to 24.4.0 #12594

Merged

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Use ruff format #12478

Use ruff format #12478

sbidoul commented Jan 15, 2024

pfmoore left a comment

pfmoore Jan 15, 2024

notatallshaw Jan 15, 2024

pfmoore Jan 15, 2024

sbidoul Jan 15, 2024

notatallshaw Jan 15, 2024

pfmoore Jan 15, 2024

pfmoore Jan 15, 2024

pfmoore Jan 15, 2024

pfmoore Jan 15, 2024

pfmoore Jan 15, 2024

pfmoore Jan 15, 2024

sbidoul Jan 15, 2024 •

edited

Loading

pfmoore Jan 15, 2024

pfmoore Jan 15, 2024

notatallshaw Jan 15, 2024 •

edited

Loading

pfmoore Jan 15, 2024

pfmoore Jan 15, 2024

sbidoul Jan 15, 2024

pfmoore Jan 15, 2024

hugovk Jan 15, 2024

pfmoore Jan 15, 2024

notatallshaw Jan 15, 2024

pfmoore commented Jan 15, 2024

notatallshaw commented Jan 15, 2024 •

edited

Loading

sbidoul commented Jan 15, 2024

notatallshaw commented Jan 15, 2024 •

edited

Loading

Use ruff format #12478

Are you sure you want to change the base?

Use ruff format #12478

Conversation

sbidoul commented Jan 15, 2024

pfmoore left a 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

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

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

sbidoul Jan 15, 2024 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

notatallshaw Jan 15, 2024 • 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

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

pfmoore commented Jan 15, 2024

notatallshaw commented Jan 15, 2024 • edited Loading

sbidoul commented Jan 15, 2024

notatallshaw commented Jan 15, 2024 • edited Loading

sbidoul Jan 15, 2024 •

edited

Loading

notatallshaw Jan 15, 2024 •

edited

Loading

notatallshaw commented Jan 15, 2024 •

edited

Loading

notatallshaw commented Jan 15, 2024 •

edited

Loading