Skip to content

refactor: format log messages lazily with %s arguments and enable ruff G004 - #1234

Merged
vdusek merged 2 commits into
masterfrom
refactor/lazy-log-formatting
Nov 6, 2026
Merged

vdusek merged 2 commits into
masterfrom
refactor/lazy-log-formatting

Conversation

@vdusek

@vdusek vdusek commented Nov 6, 2026

Copy link
Copy Markdown
Contributor

Logging calls now pass their values as %-style arguments instead of f-strings, so the message is only formatted when a handler actually emits the record. This also gives log aggregators a stable record.msg template to group by. Ruff's G004 is removed from the ignore list so new f-string log calls fail lint.

The rendered messages are the same as before. One unit test compared record.msg against the formatted text and now uses record.getMessage().

✍️ Drafted by Claude Code

@vdusek vdusek added adhoc Ad-hoc unplanned task added during the sprint. t-tooling Issues with this label are in the ownership of the tooling team. labels Nov 6, 2026
@vdusek vdusek self-assigned this Nov 6, 2026
@codecov

codecov Bot commented Nov 6, 2026 •

Copy link
Copy Markdown

Codecov Report

❌ Patch coverage is 77.14286% with 8 lines in your changes missing coverage. Please review.
✅ Project coverage is 93.81%. Comparing base (ea4d104) to head (cb4ee76).
⚠️ Report is 2 commits behind head on master.

Files with missing lines Patch % Lines
src/apify/scrapy/scheduler.py 57.14% 3 Missing ⚠️
...age_clients/_apify/_request_queue_shared_client.py 66.66% 2 Missing ⚠️
...c/apify/storage_clients/_apify/_alias_resolving.py 0.00% 1 Missing ⚠️
...age_clients/_apify/_request_queue_single_client.py 50.00% 1 Missing ⚠️
...ge_clients/_file_system/_key_value_store_client.py 0.00% 1 Missing ⚠️
Additional details and impacted files
@@           Coverage Diff           @@
##           master    #1234   +/-   ##
=======================================
  Coverage   93.81%   93.81%           
=======================================
  Files          54       54           
  Lines        3800     3800           
=======================================
  Hits         3565     3565           
  Misses        235      235           
Flag Coverage Δ
e2e ?
integration 51.39% <5.71%> (ø)
unit 91.00% <74.28%> (ø)

Flags with carried forward coverage won't be shown. Click here to find out more.

☔ View full report in Codecov by Harness.
📢 Have feedback on the report? Share it here.

🚀 New features to boost your workflow:
  • ❄️ Test Analytics: Detect flaky tests, report on failures, and find test suite problems.
  • 📦 JS Bundle Analysis: Save yourself from yourself by tracking and limiting bundle sizes in JS merges.

@vdusek
vdusek marked this pull request as ready for review November 6, 2026 07:00
@apify-service-account apify-service-account added the tested Temporary label used only programatically for some analytics. label Nov 6, 2026

@apify-factory apify-factory left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

Looks good, nothing blocking.

🏭 Factory review — ℹ️ not auto-approved

This change was reviewed but cannot be auto-approved: a gate failed, so the review cannot end in an approval.

check result
Static gates ❌ 13/16 — max-files, file-extensions, deny-globs
Review findings ✅ none
Reviewer escalation ✅ none
Juror 1 — claude-sonnet-5-5, adversarial ⏭️ skipped — a gate failed
Why it was not auto-approved

Failed gates:

  • max-files: 20 changed files, limit 10
  • file-extensions: disallowed extension: pyproject.toml
  • deny-globs: src/apify/_charging.py matches src/apify/_charging.py

The comments above are the review itself; this section only explains the approval decision. Push a change and request @apify-factory again to re-run it, or ask a human to review.

Reviewed cb4ee7621 · workflow run

@vdusek

vdusek commented Nov 6, 2026

Copy link
Copy Markdown
Contributor Author

e2e fail because of new version of w3lib (transitive dependency), will be resolved in #1235

vdusek added a commit that referenced this pull request Nov 6, 2026
…ile with uv (#1235)

The E2E test Actors installed unpinned dependencies, so a new upstream
release could break every open PR at once. `w3lib` 2.6.0 did that (for
example on #1234).

- Install the E2E test Actor dependencies with uv, pinned to the
project's `uv.lock`.
- Build the Actor image in two stages: the builder installs the SDK from
source with its dependencies into a virtual environment, and the runner
copies it over. The Playwright image works the same way.
- Pass the Python version to the build as a required `PYTHON_VERSION`
build argument.
- Rename the test Actor package from `src` to `actor`.
- Run the E2E tests on Python 3.11 and 3.15.
- Make the Scrapy E2E test pass with both `w3lib` 2.5.0 and 2.6.0.
- Drop the unused `build` dev dependency.

*✍️ Drafted by Claude Code*
@vdusek
vdusek merged commit 347d7b2 into master Nov 6, 2026
40 of 42 checks passed
@vdusek
vdusek deleted the refactor/lazy-log-formatting branch November 6, 2026 12:22
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

adhoc Ad-hoc unplanned task added during the sprint. t-tooling Issues with this label are in the ownership of the tooling team. tested Temporary label used only programatically for some analytics.

Projects

None yet

Development

Successfully merging this pull request may close these issues.

4 participants