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

chore: do not use f-strings in logging statements #32

Merged
Merged
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
2 changes: 2 additions & 0 deletions .git-blame-ignore-revs
Original file line number Diff line number Diff line change
@@ -0,0 +1,2 @@
# Enable Ruff G004 to enforce logging best practices
4c3043200ca15ade613fa3e3f8ca84bbe3522f21
Copy link
Member

Choose a reason for hiding this comment

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

What's this file for?

Copy link
Member Author

Choose a reason for hiding this comment

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

When we make formatting/style changes in the code that are not useful for the git blame, we can ignore the commits (revisions) in which the changes were made in the relevant files to keep their history clean.

More on this here: https://graphite.dev/guides/git-blame-ignore-revs

https://github.com/orgs/community/discussions/5033

Copy link
Member Author

Choose a reason for hiding this comment

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

Now that I fixed it – if you check any of the files, say, main/pyodide_build/buildall.py (blame), it wouldn't show the formatting change in the history (by default).

2 changes: 1 addition & 1 deletion pyodide_build/_py_compile.py
Original file line number Diff line number Diff line change
Expand Up @@ -114,7 +114,7 @@ def _compile(
output_name = output_path.name

with set_log_level(logger, verbose):
logger.debug(f"Running py-compile on {input_path} to {output_path}")
logger.debug("Running py-compile on %s to %s", input_path, output_path)

if compression_level > 0:
compression = zipfile.ZIP_DEFLATED
Expand Down
4 changes: 2 additions & 2 deletions pyodide_build/bash_runner.py
Original file line number Diff line number Diff line change
Expand Up @@ -76,11 +76,11 @@ def run(
if cwd is None:
cwd = Path.cwd()
cwd = Path(cwd).absolute()
logger.info(f"Running {script_name} in {str(cwd)}")
logger.info("Running %s in %s", script_name, str(cwd))
opts["cwd"] = cwd
result = self.run_unchecked(cmd, **opts)
if result.returncode != 0:
logger.error(f"ERROR: {script_name} failed")
logger.error("ERROR: %s failed", script_name)
logger.error(textwrap.indent(cmd, " "))
exit_with_stdio(result)
return result
Expand Down
18 changes: 9 additions & 9 deletions pyodide_build/buildall.py
Original file line number Diff line number Diff line change
Expand Up @@ -434,7 +434,7 @@ def generate_dependency_graph(

if disabled_packages:
logger.warning(
f"The following packages are disabled: {', '.join(disabled_packages)}"
"The following packages are disabled: %s", ", ".join(disabled_packages)
)

return pkg_map
Expand Down Expand Up @@ -688,17 +688,17 @@ def build_from_graph(

if already_built:
logger.info(
"The following packages are already built: "
f"[bold]{format_name_list(sorted(already_built))}[/bold]"
"The following packages are already built: [bold]%s[/bold]",
format_name_list(sorted(already_built)),
)
if not needs_build:
logger.success("All packages already built. Quitting.")
return

sorted_needs_build = sorted(needs_build)
logger.info(
"Building the following packages: "
f"[bold]{format_name_list(sorted_needs_build)}[/bold]"
"Building the following packages: [bold]%s[/bold]",
format_name_list(sorted_needs_build),
)
build_state = _GraphBuilder(pkg_map, build_args, build_dir, set(needs_build))
try:
Expand Down Expand Up @@ -850,14 +850,14 @@ def copy_logs(pkg_map: dict[str, BasePackage], log_dir: Path) -> None:
"""

log_dir.mkdir(exist_ok=True, parents=True)
logger.info(f"Copying build logs to {log_dir}")
logger.info("Copying build logs to %s", log_dir)

for pkg in pkg_map.values():
log_file = pkg.pkgdir / "build.log"
if log_file.exists():
shutil.copy(log_file, log_dir / f"{pkg.name}.log")
else:
logger.warning(f"Warning: {pkg.name} has no build log")
logger.warning("Warning: %s has no build log", pkg.name)


def install_packages(
Expand All @@ -881,7 +881,7 @@ def install_packages(

output_dir.mkdir(exist_ok=True, parents=True)

logger.info(f"Copying built packages to {output_dir}")
logger.info("Copying built packages to %s", output_dir)
copy_packages_to_dist_dir(
pkg_map.values(),
output_dir,
Expand All @@ -890,7 +890,7 @@ def install_packages(
)

lockfile_path = output_dir / "pyodide-lock.json"
logger.info(f"Writing pyodide-lock.json to {lockfile_path}")
logger.info("Writing pyodide-lock.json to %s", lockfile_path)

package_data = generate_lockfile(output_dir, pkg_map)
package_data.to_json(lockfile_path)
Expand Down
10 changes: 5 additions & 5 deletions pyodide_build/buildpkg.py
Original file line number Diff line number Diff line change
Expand Up @@ -130,7 +130,7 @@ def build(self) -> None:

t0 = datetime.now()
timestamp = t0.strftime("%Y-%m-%d %H:%M:%S")
logger.info(f"[{timestamp}] Building package {self.name}...")
logger.info("[%s] Building package %s...", timestamp, self.name)
success = True
try:
self._build()
Expand Down Expand Up @@ -439,7 +439,7 @@ def _package_wheel(
# Retag platformed wheels to pyodide
wheel = retag_wheel(wheel, wheel_platform())

logger.info(f"Unpacking wheel to {str(wheel)}")
logger.info("Unpacking wheel to %s", str(wheel))

name, ver, _ = wheel.name.split("-", 2)

Expand Down Expand Up @@ -509,7 +509,7 @@ def _patch(self) -> None:
cwd=self.src_extract_dir,
)
if result.returncode != 0:
logger.error(f"ERROR: Patch {patch_abspath} failed")
logger.error("ERROR: Patch %s failed", patch_abspath)
exit_with_stdio(result)

# Add any extra files
Expand Down Expand Up @@ -612,7 +612,7 @@ def copy_sharedlibs(
logger.info("Copied shared libraries:")
for lib, path in dep_map_new.items():
original_path = dep_map[lib]
logger.info(f" {original_path} -> {path}")
logger.info(" %s -> %s", original_path, path)

return dep_map_new

Expand Down Expand Up @@ -695,7 +695,7 @@ def needs_rebuild(
packaged_token = buildpath / ".packaged"
if not packaged_token.is_file():
logger.debug(
f"{pkg_root} needs rebuild because {packaged_token} does not exist"
"%s needs rebuild because %s does not exist", pkg_root, packaged_token
)
return True

Expand Down
4 changes: 2 additions & 2 deletions pyodide_build/cli/skeleton.py
Original file line number Diff line number Diff line change
Expand Up @@ -80,10 +80,10 @@ def new_recipe_pypi(
update_patched=update_patched,
)
except mkpkg.MkpkgFailedException as e:
logger.error(f"{name} update failed: {e}")
logger.error("%s update failed: %s", name, e)
sys.exit(1)
except mkpkg.MkpkgSkipped as e:
logger.warning(f"{name} update skipped: {e}")
logger.warning("%s update skipped: %s", name, e)
except Exception:
print(name)
raise
Expand Down
8 changes: 4 additions & 4 deletions pyodide_build/common.py
Original file line number Diff line number Diff line change
Expand Up @@ -101,7 +101,7 @@ def _has_python_file(subdir: zipfile.Path) -> bool:

if not top_level_imports:
logger.warning(
f"WARNING: failed to parse top level import name from {whlfile}."
"WARNING: failed to parse top level import name from %s.", whlfile
)
return None

Expand Down Expand Up @@ -293,7 +293,7 @@ def unpack_wheel(wheel_path: Path, target_dir: Path | None = None) -> None:
encoding="utf-8",
)
if result.returncode != 0:
logger.error(f"ERROR: Unpacking wheel {wheel_path.name} failed")
logger.error("ERROR: Unpacking wheel %s failed", wheel_path.name)
exit_with_stdio(result)


Expand All @@ -306,7 +306,7 @@ def pack_wheel(wheel_dir: Path, target_dir: Path | None = None) -> None:
encoding="utf-8",
)
if result.returncode != 0:
logger.error(f"ERROR: Packing wheel {wheel_dir} failed")
logger.error("ERROR: Packing wheel %s failed", wheel_dir)
exit_with_stdio(result)


Expand Down Expand Up @@ -348,7 +348,7 @@ def retag_wheel(wheel_path: Path, platform: str) -> Path:
capture_output=True,
)
if result.returncode != 0:
logger.error(f"ERROR: Retagging wheel {wheel_path} to {platform} failed")
logger.error("ERROR: Retagging wheel %s to %s failed", wheel_path, platform)
exit_with_stdio(result)
return wheel_path.parent / result.stdout.splitlines()[-1].strip()

Expand Down
3 changes: 2 additions & 1 deletion pyodide_build/config.py
Original file line number Diff line number Diff line change
Expand Up @@ -110,7 +110,8 @@ def _load_config_file(
for key, v in configs["tool"]["pyodide"]["build"].items():
if key not in OVERRIDABLE_BUILD_KEYS:
logger.warning(
f"WARNING: The provided build key {key} is either invalid or not overridable, hence ignored."
"WARNING: The provided build key %s is either invalid or not overridable, hence ignored.",
key,
)
continue
build_config[key] = _environment_substitute_str(v, env)
Expand Down
17 changes: 10 additions & 7 deletions pyodide_build/mkpkg.py
Original file line number Diff line number Diff line change
Expand Up @@ -151,7 +151,7 @@ def make_package(
Creates a template that will work for most pure Python packages,
but will have to be edited for more complex things.
"""
logger.info(f"Creating meta.yaml package for {package}")
logger.info("Creating meta.yaml package for %s", package)

yaml = YAML()

Expand Down Expand Up @@ -226,7 +226,7 @@ def update_package(

meta_path = root / package / "meta.yaml"
if not meta_path.exists():
logger.error(f"{meta_path} does not exist")
logger.error("%s does not exist", meta_path)
raise MkpkgFailedException(f"{package} recipe not found at {meta_path}")

yaml_content = yaml.load(meta_path.read_bytes())
Expand Down Expand Up @@ -281,16 +281,19 @@ def update_package(
return

logger.info(
f"{package} is out of date:"
f" either {local_ver} < {pypi_ver}"
f" or checksums might have mismatched: received {sha256} against local {sha256_local} 🚨"
"%s is out of date: either %s < %s or checksums might have mismatched: received %s against local %s 🚨",
package,
local_ver,
pypi_ver,
sha256,
sha256_local,
)

if yaml_content["source"].get("patches"):
if update_patched:
logger.warning(
f"Pyodide applies patches to {package}. Update the "
"patches (if needed) to avoid build failing."
"Pyodide applies patches to %s. Update the patches (if needed) to avoid build failing.",
package,
)
else:
raise MkpkgFailedException(
Expand Down
2 changes: 1 addition & 1 deletion pyodide_build/out_of_tree/pypi.py
Original file line number Diff line number Diff line change
Expand Up @@ -95,7 +95,7 @@ def _get_built_wheel_internal(url):
source_path = build_path / files[0]
else:
source_path = build_path
logger.info(f"Building wheel for {gz_name}...")
logger.info("Building wheel for %s...", gz_name)
with (
tempfile.NamedTemporaryFile(mode="w+") as logfile,
stream_redirected(to=logfile, stream=sys.stdout),
Expand Down
4 changes: 2 additions & 2 deletions pyodide_build/out_of_tree/venv.py
Original file line number Diff line number Diff line change
Expand Up @@ -254,11 +254,11 @@ def install_stdlib(venv_bin: Path) -> None:

def create_pyodide_venv(dest: Path) -> None:
"""Create a Pyodide virtualenv and store it into dest"""
logger.info(f"Creating Pyodide virtualenv at {dest}")
logger.info("Creating Pyodide virtualenv at %s", dest)
from virtualenv import session_via_cli

if dest.exists():
logger.error(f"ERROR: dest directory '{dest}' already exists")
logger.error("ERROR: dest directory '%s' already exists", dest)
sys.exit(1)

interp_path = pyodide_dist_dir() / "python"
Expand Down
5 changes: 3 additions & 2 deletions pyodide_build/recipe.py
Original file line number Diff line number Diff line change
Expand Up @@ -87,8 +87,9 @@ def load_recipes(

elif name_or_tag in ("core", "min-scipy-stack"):
logger.warning(
f"Using meta package without the 'tag:' prefix is deprecated,"
f" use 'tag:{name_or_tag}' instead."
"Using meta package without the 'tag:' prefix is deprecated, "
"use 'tag:%s' instead.",
name_or_tag,
)
for recipe in tagged_recipes[name_or_tag]:
recipes[recipe.package.name] = recipe.model_copy(deep=True)
Expand Down
5 changes: 3 additions & 2 deletions pyodide_build/xbuildenv.py
Original file line number Diff line number Diff line change
Expand Up @@ -106,7 +106,7 @@ def use_version(self, version: str) -> None:
version
The version of xbuildenv to use.
"""
logger.info(f"Using Pyodide cross-build environment version: {version}")
logger.info("Using Pyodide cross-build environment version: %s", version)

version_path = self._path_for_version(version)
if not version_path.exists():
Expand Down Expand Up @@ -358,7 +358,8 @@ def _create_package_index(self, xbuildenv_pyodide_root: Path, version: str) -> N

if not lockfile_path.exists():
logger.warning(
f"Pyodide lockfile not found at {lockfile_path}. Skipping PyPI index creation"
"Pyodide lockfile not found at %s. Skipping PyPI index creation",
lockfile_path,
)
return

Expand Down
4 changes: 4 additions & 0 deletions pyproject.toml
Original file line number Diff line number Diff line change
Expand Up @@ -125,7 +125,11 @@ lint.select = [
"PLC", # pylint conventions
"PLE", # pylint errors
"UP", # pyupgrade
"G004", # f-string logging should be avoided
]

lint.logger-objects = ["pyodide_build.logger.logger"]

lint.ignore = ["E402", "E501", "E731", "E741", "UP038"]
# line-length = 219 # E501: Recommended goal is 88 to match black
target-version = "py311"
Expand Down
Loading