Skip to content

Commit

Permalink
Do not use f-strings in log calls. (#131)
Browse files Browse the repository at this point in the history
* Do not use f-strings in log calls.

It is in general bad practice; in particular if one want a structured
logging handler that store the template string and the values separately, using
f-strings makes it not possible.

I belive there is also a question of actually calling the formatter on
the interpolated values which is not done if the loging level is low
enough.

WHile it might not be too critical for micropip, I belive it is good
practive to show the example.

* Enable ruff lint and fix 2 extras logging using + concatenation.

* forgotten %
  • Loading branch information
Carreau authored Aug 26, 2024
1 parent 8807c59 commit 805140f
Show file tree
Hide file tree
Showing 4 changed files with 17 additions and 14 deletions.
18 changes: 10 additions & 8 deletions micropip/_commands/uninstall.py
Original file line number Diff line number Diff line change
Expand Up @@ -39,15 +39,15 @@ def uninstall(packages: str | list[str], *, verbose: bool | int = False) -> None
dist = importlib.metadata.distribution(package)
distributions.append(dist)
except importlib.metadata.PackageNotFoundError:
logger.warning(f"Skipping '{package}' as it is not installed.")
logger.warning("Skipping '%s' as it is not installed.", package)

for dist in distributions:
# Note: this value needs to be retrieved before removing files, as
# dist.name uses metadata file to get the name
name = dist.name
version = dist.version

logger.info(f"Found existing installation: {name} {version}")
logger.info("Found existing installation: %s %s", name, version)

root = get_root(dist)
files = get_files_in_distribution(dist)
Expand All @@ -64,7 +64,9 @@ def uninstall(packages: str | list[str], *, verbose: bool | int = False) -> None
continue

logger.warning(
f"A file '{file}' listed in the metadata of '{name}' does not exist.",
"A file '%s' listed in the metadata of '%s' does not exist.",
file,
name,
)

continue
Expand All @@ -80,18 +82,18 @@ def uninstall(packages: str | list[str], *, verbose: bool | int = False) -> None
directory.rmdir()
except OSError:
logger.warning(
f"A directory '{directory}' is not empty after uninstallation of '{name}'. "
"A directory '%s' is not empty after uninstallation of '%s'. "
"This might cause problems when installing a new version of the package. ",
directory,
name,
)

if hasattr(loadedPackages, name):
delattr(loadedPackages, name)
else:
# This should not happen, but just in case
logger.warning(
f"a package '{name}' was not found in loadedPackages.",
)
logger.warning("a package '%s' was not found in loadedPackages.", name)

logger.info(f"Successfully uninstalled {name}-{version}")
logger.info("Successfully uninstalled %s-%s", name, version)

importlib.invalidate_caches()
4 changes: 2 additions & 2 deletions micropip/install.py
Original file line number Diff line number Diff line change
Expand Up @@ -155,7 +155,7 @@ async def install(
]

if package_names:
logger.info("Installing collected packages: " + ", ".join(package_names))
logger.info("Installing collected packages: %s", ", ".join(package_names))

wheel_promises: list[Coroutine[Any, Any, None] | asyncio.Task[Any]] = []
# Install built-in packages
Expand All @@ -182,6 +182,6 @@ async def install(
]

if packages:
logger.info("Successfully installed " + ", ".join(packages))
logger.info("Successfully installed %s", ", ".join(packages))

importlib.invalidate_caches()
8 changes: 4 additions & 4 deletions micropip/transaction.py
Original file line number Diff line number Diff line change
Expand Up @@ -140,7 +140,7 @@ def eval_marker(e: dict[str, str]) -> bool:

satisfied, ver = self.check_version_satisfied(req)
if satisfied:
logger.info(f"Requirement already satisfied: {req} ({ver})")
logger.info("Requirement already satisfied: %s (%s)", req, ver)
return

try:
Expand Down Expand Up @@ -193,7 +193,7 @@ async def _add_requirement_from_package_index(self, req: Requirement):
# installed the wheel?
satisfied, ver = self.check_version_satisfied(req)
if satisfied:
logger.info(f"Requirement already satisfied: {req} ({ver})")
logger.info("Requirement already satisfied: %s (%s)", req, ver)

await self.add_wheel(wheel, req.extras, specifier=str(req.specifier))

Expand Down Expand Up @@ -228,8 +228,8 @@ async def add_wheel(
version=str(wheel.version),
)

logger.info(f"Collecting {wheel.name}{specifier}")
logger.info(f" Downloading {wheel.url.split('/')[-1]}")
logger.info("Collecting %s%s", wheel.name, specifier)
logger.info(" Downloading %s", wheel.url.split("/")[-1])

await wheel.download(self.fetch_kwargs)
if self.deps:
Expand Down
1 change: 1 addition & 0 deletions pyproject.toml
Original file line number Diff line number Diff line change
Expand Up @@ -50,6 +50,7 @@ select = [
"UP", # pyupgrade
"I", # isort
"PGH", # pygrep-hooks
"G", # flake8-logging-format
]
# Remove E999 once pattern matching is supported
# https://github.com/charliermarsh/ruff/issues/282
Expand Down

0 comments on commit 805140f

Please sign in to comment.