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.

Sign up for GitHub

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

Use ruff format #12478

Draft
wants to merge 2 commits into
base: main
Choose a base branch
from
Draft

Use ruff format #12478

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
2 commits
Select commit Hold shift + click to select a range
2530ce8
Use ruff-format
sbidoul Jan 15, 2024
f41c836
Improve typing in test_utils
sbidoul Jan 15, 2024
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
8 changes: 2 additions & 6 deletions .pre-commit-config.yaml
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -16,16 +16,12 @@ repos:
- id: trailing-whitespace
exclude: .patch

- repo: https://github.com/psf/black-pre-commit-mirror
rev: 23.12.1
hooks:
- id: black

- repo: https://github.com/astral-sh/ruff-pre-commit
rev: v0.1.9
rev: v0.1.13
hooks:
- id: ruff
args: [--fix, --exit-non-zero-on-fix]
- id: ruff-format

- repo: https://github.com/pre-commit/mirrors-mypy
rev: v1.8.0
Expand Down
1 change: 1 addition & 0 deletions pyproject.toml
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -149,6 +149,7 @@ ignore = [
"B020",
"B904", # Ruff enables opinionated warnings by default
"B905", # Ruff enables opinionated warnings by default
"ISC001", # Avoid conflict with ruff formatter
]
target-version = "py37"
line-length = 88
Expand Down
2 changes: 1 addition & 1 deletion src/pip/_internal/cli/base_command.py
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -172,7 +172,7 @@ def _main(self, args: List[str]) -> int:
options.cache_dir = None

def intercepts_unhandled_exc(
run_func: Callable[..., int]
run_func: Callable[..., int],
) -> Callable[..., int]:
@functools.wraps(run_func)
def exc_logging_wrapper(*args: Any) -> int:
Expand Down
5 changes: 3 additions & 2 deletions src/pip/_internal/commands/freeze.py
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -71,8 +71,9 @@ def add_options(self) -> None:
dest="freeze_all",
action="store_true",
help=(
"Do not skip these packages in the output:"
" {}".format(", ".join(_dev_pkgs()))
"Do not skip these packages in the output: {}".format(
", ".join(_dev_pkgs())
)
),
)
self.cmd_opts.add_option(
Expand Down
4 changes: 2 additions & 2 deletions src/pip/_internal/commands/install.py
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -306,8 +306,8 @@ def run(self, options: Values, args: List[str]) -> int:
options.target_dir = os.path.abspath(options.target_dir)
if (
# fmt: off
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)
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

# fmt: on
):
raise CommandError(
Expand Down
3 changes: 2 additions & 1 deletion src/pip/_internal/metadata/_json.py
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -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
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

mypy is happy with it, so it should be fine

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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).

]
else:
value = sanitise_header(msg.get(field)) # type: ignore
Expand Down
2 changes: 1 addition & 1 deletion src/pip/_internal/req/req_uninstall.py
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -38,7 +38,7 @@ def _script_names(


def _unique(
fn: Callable[..., Generator[Any, None, None]]
fn: Callable[..., Generator[Any, None, None]],
) -> Callable[..., Generator[Any, None, None]]:
@functools.wraps(fn)
def unique(*args: Any, **kw: Any) -> Generator[Any, None, None]:
Expand Down
4 changes: 1 addition & 3 deletions tests/functional/test_build_env.py
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -54,9 +54,7 @@ def run_with_build_env(

with global_tempdir_manager():
build_env = BuildEnvironment()
""".format(
scratch=str(script.scratch_path)
)
""".format(scratch=str(script.scratch_path))
)
+ indent(dedent(setup_script_contents), " ")
+ indent(
Expand Down
7 changes: 2 additions & 5 deletions tests/functional/test_completion.py
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -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
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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 🙄



def test_completion_for_un_snippet(autocomplete: DoAutocomplete) -> None:
Expand Down Expand Up @@ -251,10 +251,7 @@ def test_completion_files_after_option(
), "autocomplete function could not complete <dir> after options in command"
assert not any(
out in res.stdout for out in (os.path.join("REPLAY", ""), "README.txt")
), (
"autocomplete function completed <file> or <dir> that "
"should not be completed"
)
), "autocomplete function completed <file> or <dir> that should not be completed"
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

if sys.platform != "win32":
return
assert (
Expand Down
5 changes: 1 addition & 4 deletions tests/functional/test_install_user.py
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -135,10 +135,7 @@ def test_install_user_conflict_in_usersite(
dist_info_folder = script.user_site / "INITools-0.1.dist-info"
initools_v3_file = (
# file only in 0.3
script.base_path
/ script.user_site
/ "initools"
/ "configparser.py"
script.base_path / script.user_site / "initools" / "configparser.py"
)
result2.did_create(dist_info_folder)
assert not isfile(initools_v3_file), initools_v3_file
Expand Down
10 changes: 4 additions & 6 deletions tests/functional/test_new_resolver.py
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -414,9 +414,8 @@ def test_new_resolver_requires_python_error(script: PipTestEnvironment) -> None:
expect_error=True,
)

message = (
"Package 'base' requires a different Python: "
"{}.{}.{} not in '<2'".format(*sys.version_info[:3])
message = "Package 'base' requires a different Python: {}.{}.{} not in '<2'".format(
*sys.version_info[:3]
)
assert message in result.stderr, str(result)

Expand Down Expand Up @@ -1148,9 +1147,8 @@ def test_new_resolver_no_deps_checks_requires_python(
expect_error=True,
)

message = (
"Package 'base' requires a different Python: "
"{}.{}.{} not in '<2'".format(*sys.version_info[:3])
message = "Package 'base' requires a different Python: {}.{}.{} not in '<2'".format(
*sys.version_info[:3]
)
assert message in result.stderr

Expand Down
2 changes: 1 addition & 1 deletion tests/lib/server.py
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -48,7 +48,7 @@ def make_environ(self) -> Dict[str, Any]:


def _mock_wsgi_adapter(
mock: Callable[["WSGIEnvironment", "StartResponse"], "WSGIApplication"]
mock: Callable[["WSGIEnvironment", "StartResponse"], "WSGIApplication"],
) -> "WSGIApplication":
"""Uses a mock to record function arguments and provide
the actual function that should respond.
Expand Down
2 changes: 1 addition & 1 deletion tests/unit/resolution_resolvelib/test_resolver.py
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -34,7 +34,7 @@ def resolver(preparer: RequirementPreparer, finder: PackageFinder) -> Resolver:


def _make_graph(
edges: List[Tuple[Optional[str], Optional[str]]]
edges: List[Tuple[Optional[str], Optional[str]]],
) -> "DirectedGraph[Optional[str]]":
"""Build graph from edge declarations."""

Expand Down
9 changes: 6 additions & 3 deletions tests/unit/test_configuration.py
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -237,9 +237,12 @@ def test_user_modification(self) -> None:

# get the path to user config file
assert mymock.call_count == 1
assert mymock.call_args[0][0] == (
# Use new config file
get_configuration_files()[kinds.USER][1]
assert (
mymock.call_args[0][0]
== (
# Use new config file
get_configuration_files()[kinds.USER][1]
)
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This is definitely worse than the black choice.

)

def test_global_modification(self) -> None:
Expand Down
7 changes: 4 additions & 3 deletions tests/unit/test_utils.py
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -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)
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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?

Copy link
Member Author

@sbidoul sbidoul Jan 15, 2024 •
edited
Loading

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.


mock_func = Mock()

Expand All @@ -264,7 +263,9 @@ def test_rmtree_errorhandler_reraises_error(tmpdir: Path) -> None:
# Tuple[None, None, None]]"; expected "Tuple[Type[BaseException],
# BaseException, TracebackType]"
rmtree_errorhandler(
mock_func, path, sys.exc_info() # type: ignore[arg-type]
mock_func,
subdir_path,
sys.exc_info(), # type: ignore[arg-type]
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

Copy link
Member

@notatallshaw notatallshaw Jan 15, 2024 •
edited
Loading

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I added a note about this case to that issue.

)

mock_func.assert_not_called()
Expand Down
34 changes: 19 additions & 15 deletions tools/release/__init__.py
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -105,13 +105,8 @@ def update_version_file(version: str, filepath: str) -> None:


def create_git_tag(session: Session, tag_name: str, *, message: str) -> None:
session.run(
# fmt: off
"git", "tag", "-m", message, tag_name,
# fmt: on
external=True,
silent=True,
)
cmd = ["git", "tag", "-m", message, tag_name]
session.run(*cmd, external=True, silent=True)
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Yes I should have done that in another commit.



def get_next_development_version(version: str) -> str:
Expand Down Expand Up @@ -173,12 +168,17 @@ def isolated_temporary_checkout(
git_checkout_dir = tmp_dir / f"pip-build-{target_ref}"
nox_session.run(
# fmt: off
"git", "clone",
"--depth", "1",
"--config", "core.autocrlf=false",
"--branch", str(target_ref),
"git",
"clone",
"--depth",
"1",
"--config",
"core.autocrlf=false",
"--branch",
str(target_ref),
"--",
".", str(git_checkout_dir),
".",
str(git_checkout_dir),
# fmt: on
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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).

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

external=True,
silent=True,
Expand All @@ -191,9 +191,13 @@ def get_git_untracked_files() -> Iterator[str]:
"""List all local file paths that aren't tracked by Git."""
git_ls_files_cmd = (
# fmt: off
"git", "ls-files",
"--ignored", "--exclude-standard",
"--others", "--", ".",
"git",
"ls-files",
"--ignored",
"--exclude-standard",
"--others",
"--",
".",
# fmt: on
)
# session.run doesn't seem to return any output:
Expand Down
Loading