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.

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

Apply repo-review suggestions #1519

Open
wants to merge 2 commits into
base: main
Choose a base branch
from
Open
Show file tree
Hide file tree
Changes from all commits
Commits
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
7 changes: 4 additions & 3 deletions pyproject.toml
Original file line number Diff line number Diff line change
Expand Up @@ -62,9 +62,6 @@ version.source = "vcs"

[tool.ruff]
line-length = 121
src = [
"src",
]
lint.extend-select = [
"A",
"B",
Expand Down Expand Up @@ -122,6 +119,10 @@ title_format = "## [{version}](https://github.com/pypa/pipx/tree/{version}) - {p
issue_format = "[#{issue}](https://github.com/pypa/pipx/issues/{issue})"
package = "pipx"

[tool.mypy]
warn_unreachable = true
enable_error_code = [ "ignore-without-code", "redundant-expr", "truthy-bool" ]

[[tool.mypy.overrides]]
module = [
"pycowsay.*",
Expand Down
2 changes: 1 addition & 1 deletion src/pipx/animate.py
Original file line number Diff line number Diff line change
Expand Up @@ -101,7 +101,7 @@ def print_animation(
def win_cursor(visible: bool) -> None:
if sys.platform != "win32": # hello mypy
return
ci = _CursorInfo()
ci = _CursorInfo() # type: ignore[unreachable]
handle = ctypes.windll.kernel32.GetStdHandle(-11)
ctypes.windll.kernel32.GetConsoleCursorInfo(handle, ctypes.byref(ci))
ci.visible = visible
Expand Down
2 changes: 1 addition & 1 deletion src/pipx/commands/environment.py
Original file line number Diff line number Diff line change
Expand Up @@ -38,7 +38,7 @@ def environment(value: str) -> ExitCode:
"PIPX_HOME_ALLOW_SPACE": str(paths.ctx.allow_spaces_in_home_path).lower(),
}
if value is None:
print("Environment variables (set by user):")
print("Environment variables (set by user):") # type: ignore[unreachable]
Copy link
Contributor Author

Choose a reason for hiding this comment

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

value is not Optional, so in theory it cannot be None.

Do we still want to defend against an actual None?

print("")
for env_variable in ENVIRONMENT_VARIABLES:
env_value = os.getenv(env_variable, "")
Expand Down
2 changes: 1 addition & 1 deletion src/pipx/commands/inject.py
Original file line number Diff line number Diff line change
Expand Up @@ -32,7 +32,7 @@ def inject_dep(
) -> bool:
logger.debug("Injecting package %s", package_spec)

if not venv_dir.exists() or not next(venv_dir.iterdir()):
if not venv_dir.exists() or next(venv_dir.iterdir(), None) is None:
Copy link
Contributor Author

Choose a reason for hiding this comment

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

venv_dir.iterdir() yields Path which does not implement __bool__ or __len__ so it will always be true in boolean context. We might want to defend against StopIteration exceptions raised by next().

raise PipxError(
f"""
Can't inject {package_spec!r} into nonexistent Virtual Environment
Expand Down
4 changes: 2 additions & 2 deletions src/pipx/commands/run.py
Original file line number Diff line number Diff line change
Expand Up @@ -198,11 +198,11 @@ def run(
# we can't parse this as a package
package_name = app

content = None if spec is not None else maybe_script_content(app, is_path)
content = None if spec is not None else maybe_script_content(app, is_path) # type: ignore[redundant-expr]
Copy link
Contributor Author

Choose a reason for hiding this comment

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

spec is not Optional, so in theory it cannot be None.

Do we still want to defend against an actual None?

if content is not None:
run_script(content, app_args, python, pip_args, venv_args, verbose, use_cache)
else:
package_or_url = spec if spec is not None else app
package_or_url = spec if spec is not None else app # type: ignore[redundant-expr]
Copy link
Contributor Author

Choose a reason for hiding this comment

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

Same.

run_package(
package_name,
package_or_url,
Expand Down
2 changes: 1 addition & 1 deletion src/pipx/commands/uninject.py
Original file line number Diff line number Diff line change
Expand Up @@ -125,7 +125,7 @@ def uninject(
) -> ExitCode:
"""Returns pipx exit code"""

if not venv_dir.exists() or not next(venv_dir.iterdir()):
if not venv_dir.exists() or next(venv_dir.iterdir(), None) is None:
Copy link
Contributor Author

Choose a reason for hiding this comment

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

venv_dir.iterdir() yields Path which does not implement __bool__ or __len__ so it will always be true in boolean context. We might want to defend against StopIteration exceptions raised by next().

raise PipxError(f"Virtual environment {venv_dir.name} does not exist.")

venv = Venv(venv_dir, verbose=verbose)
Expand Down
4 changes: 2 additions & 2 deletions src/pipx/main.py
Original file line number Diff line number Diff line change
Expand Up @@ -277,7 +277,7 @@ def run_pipx_command(args: argparse.Namespace, subparsers: Dict[str, argparse.Ar
not args.no_cache,
)
# We should never reach here because run() is NoReturn.
return ExitCode(1)
return ExitCode(1) # type: ignore[unreachable]
elif args.command == "install":
return commands.install(
None,
Expand Down Expand Up @@ -409,7 +409,7 @@ def run_pipx_command(args: argparse.Namespace, subparsers: Dict[str, argparse.Ar
python_flag_passed=python_flag_passed,
)
elif args.command == "runpip":
if not venv_dir:
if not venv_dir: # type: ignore[truthy-bool]
Copy link
Contributor Author

Choose a reason for hiding this comment

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

In theory, venv_dir cannot be None.

Do we still want to defend against an actual None?

raise PipxError("Developer error: venv_dir is not defined.")
return commands.run_pip(package, venv_dir, args.pipargs, args.verbose)
elif args.command == "ensurepath":
Expand Down
14 changes: 7 additions & 7 deletions src/pipx/paths.py
Original file line number Diff line number Diff line change
Expand Up @@ -107,25 +107,25 @@ def shared_libs(self) -> Path:
return (self._base_shared_libs or self.home / "shared").resolve()

def make_local(self) -> None:
self._base_home = OVERRIDE_PIPX_HOME or get_expanded_environ("PIPX_HOME")
self._base_home = OVERRIDE_PIPX_HOME or get_expanded_environ("PIPX_HOME") # type: ignore[redundant-expr]
self._default_home = DEFAULT_PIPX_HOME
self._base_bin = OVERRIDE_PIPX_BIN_DIR or get_expanded_environ("PIPX_BIN_DIR")
self._base_bin = OVERRIDE_PIPX_BIN_DIR or get_expanded_environ("PIPX_BIN_DIR") # type: ignore[redundant-expr]
self._default_bin = DEFAULT_PIPX_BIN_DIR
self._base_man = OVERRIDE_PIPX_MAN_DIR or get_expanded_environ("PIPX_MAN_DIR")
self._base_man = OVERRIDE_PIPX_MAN_DIR or get_expanded_environ("PIPX_MAN_DIR") # type: ignore[redundant-expr]
self._default_man = DEFAULT_PIPX_MAN_DIR
self._base_shared_libs = OVERRIDE_PIPX_SHARED_LIBS or get_expanded_environ("PIPX_SHARED_LIBS")
self._base_shared_libs = OVERRIDE_PIPX_SHARED_LIBS or get_expanded_environ("PIPX_SHARED_LIBS") # type: ignore[redundant-expr]
self._default_log = Path(user_log_path("pipx"))
self._default_cache = Path(user_cache_path("pipx"))
self._default_trash = self._default_home / "trash"
self._fallback_home = next(iter([fallback for fallback in FALLBACK_PIPX_HOMES if fallback.exists()]), None)
self._home_exists = self._base_home is not None or any(fallback.exists() for fallback in FALLBACK_PIPX_HOMES)

def make_global(self) -> None:
self._base_home = OVERRIDE_PIPX_GLOBAL_HOME or get_expanded_environ("PIPX_GLOBAL_HOME")
self._base_home = OVERRIDE_PIPX_GLOBAL_HOME or get_expanded_environ("PIPX_GLOBAL_HOME") # type: ignore[redundant-expr]
self._default_home = DEFAULT_PIPX_GLOBAL_HOME
self._base_bin = OVERRIDE_PIPX_GLOBAL_BIN_DIR or get_expanded_environ("PIPX_GLOBAL_BIN_DIR")
self._base_bin = OVERRIDE_PIPX_GLOBAL_BIN_DIR or get_expanded_environ("PIPX_GLOBAL_BIN_DIR") # type: ignore[redundant-expr]
self._default_bin = DEFAULT_PIPX_GLOBAL_BIN_DIR
self._base_man = OVERRIDE_PIPX_GLOBAL_MAN_DIR or get_expanded_environ("PIPX_GLOBAL_MAN_DIR")
self._base_man = OVERRIDE_PIPX_GLOBAL_MAN_DIR or get_expanded_environ("PIPX_GLOBAL_MAN_DIR") # type: ignore[redundant-expr]
self._default_man = DEFAULT_PIPX_GLOBAL_MAN_DIR
self._default_log = self._default_home / "logs"
self._default_cache = self._default_home / ".cache"
Expand Down
2 changes: 1 addition & 1 deletion src/pipx/shared_libs.py
Original file line number Diff line number Diff line change
Expand Up @@ -114,7 +114,7 @@ def upgrade(self, *, pip_args: List[str], verbose: bool = False, raises: bool =
return

if pip_args is None:
pip_args = []
pip_args = [] # type: ignore[unreachable]
Copy link
Contributor Author

Choose a reason for hiding this comment

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

pip_args is not Optional, so in theory it cannot be None.

Do we still want to defend against an actual None?


logger.info(f"Upgrading shared libraries in {self.root}")

Expand Down
4 changes: 2 additions & 2 deletions tests/test_upgrade_shared.py
Original file line number Diff line number Diff line change
Expand Up @@ -23,7 +23,7 @@ def test_upgrade_shared(shared_libs, capsys, caplog):
assert "Upgrading shared libraries in" in caplog.text
assert "Already upgraded libraries in" not in caplog.text
assert shared_libs.has_been_updated_this_run is True
assert shared_libs.is_valid is True
assert shared_libs.is_valid is True # type: ignore[unreachable]
shared_libs.has_been_updated_this_run = False
assert run_pipx_cli(["upgrade-shared", "-v"]) == 0
captured = capsys.readouterr()
Expand Down Expand Up @@ -59,7 +59,7 @@ def pip_version():
assert shared_libs.is_valid is False
assert run_pipx_cli(["upgrade-shared", "-v", "--pip-args=pip==24.0"]) == 0
assert shared_libs.is_valid is True
assert pip_version() == "24.0"
assert pip_version() == "24.0" # type: ignore[unreachable]
shared_libs.has_been_updated_this_run = False # reset for next run
assert run_pipx_cli(["upgrade-shared", "-v", "--pip-args=pip==23.3.2"]) == 0
assert shared_libs.is_valid is True
Expand Down