fix(builtins): silence pklr deprecation warnings on Builtins.pkl load#880
Merged
fix(builtins): silence pklr deprecation warnings on Builtins.pkl load#880
Conversation
Contributor
|
Warning You have reached your daily quota limit. Please wait up to 24 hours and I will start processing your requests again! |
Greptile SummaryThis PR silences spurious Confidence Score: 5/5Safe to merge — patch-level dependency bump plus targeted generator fix with no behavioral changes for non-deprecated paths. All changes are narrow and well-scoped: a pklr patch bump, a generator fix that only affects deprecated alias emission, and a migration map correction. No logic regressions identified; the canonical byte_order_marker target exists and the PKL property name matches the filename stem as required by the generator template. No files require special attention. Important Files Changed
Flowchart%%{init: {'theme': 'neutral'}}%%
flowchart TD
A["Builtins.pkl load"] --> B{Identifier in deprecated aliases?}
B -- No --> C["Emit normal binding\nBuiltins[file].property"]
B -- Yes --> D["Skip normal binding"]
D --> E["Emit @Deprecated binding\npointing to canonical\nBuiltins[byte_order_marker.pkl].byte_order_marker"]
C --> F["pkl format --write"]
E --> F
G["User accesses\nBuiltins.check_byte_order_marker"] --> H["pklr >= 0.4.2:\nlazy @Deprecated eval\nfires warning ONLY here"]
I["User loads Builtins.pkl\nwithout referencing alias"] --> J["Silent — no warning"]
Reviews (4): Last reviewed commit: "fix(migrate): point pre-commit migration..." | Re-trigger Greptile |
![greptile-apps[bot]](https://avatars.githubusercontent.com/in/867647?s=60&v=4){kind=small}
Closes hk discussion #878. The auto-generated pkl/Builtins.pkl had bindings of the form check_byte_order_marker = Builtins["builtins/check_byte_order_marker.pkl"].check_byte_order_marker whose RHS reads a property annotated @deprecated. With pklr's old eager handling those warnings fired on every load; even after pklr 0.4.2's lazy @deprecated, the field-access on the deprecated stub still warns at module-evaluation time because the Builtins.pkl binding itself performs that access. Two coordinated changes: - bump pklr to 0.4.2 (lazy @deprecated, warning fires only on field access — see jdx/pklr#58, jdx/pklr#61). - have scripts/gen_builtins.py special-case the deprecated aliases: skip them in the auto-generated loop and emit them as @deprecated bindings that point at the canonical byte_order_marker step. The Builtins.pkl bindings no longer access a deprecated property at load, so the warning only fires when a user references Builtins.check_byte_order_marker / .fix_byte_order_marker directly — preserving the migration nudge. End-to-end with HK_PKL_BACKEND=pklr: - import Builtins.pkl, no use of deprecated aliases → silent. - use Builtins.check_byte_order_marker → warning fires.
jdx
force-pushed
the
fix/pkl-builtins-byte-order-marker-deprecation
branch
from
b893395 to
937be35
Compare
jdx
changed the title
![cursor[bot]](https://avatars.githubusercontent.com/in/1210556?s=60&v=4){kind=small}
There was a problem hiding this comment.
Cursor Bugbot has reviewed your changes and found 1 potential issue.
❌ Bugbot Autofix is OFF. To automatically fix reported issues with cloud agents, enable autofix in the Cursor dashboard.
Reviewed by Cursor Bugbot for commit 937be35. Configure here.
The migration map in src/cli/migrate/pre_commit.rs translated the pre-commit hook IDs `check-byte-order-marker` and `fix-byte-order-marker` to the deprecated `Builtins.*` aliases. With pklr 0.4.2's lazy @deprecated, those aliases warn the moment the user references them — so any config produced by `hk migrate pre-commit` reintroduced the noise this branch was meant to eliminate. Map both pre-commit IDs to `byte_order_marker` (the canonical step) instead, so freshly migrated configs are clean.
Merged
jdx
added a commit
that referenced
this pull request
Apr 26, 2026
### 🐛 Bug Fixes - **(builtins)** silence pklr deprecation warnings on Builtins.pkl load by [@jdx](https://github.com/jdx) in [#880](#880) - **(ci)** serialize docs lint step by [@jdx](https://github.com/jdx) in [#874](#874) - **(config)** include main pkl path in cache fresh files by [@jdx](https://github.com/jdx) in [#879](#879) - **(docs)** stack banner message and link on mobile by [@jdx](https://github.com/jdx) in [#865](#865) - **(docs)** pin banner close button to top-right corner on mobile by [@jdx](https://github.com/jdx) in [#867](#867) ### 📚 Documentation - **(site)** show release version and github stars by [@jdx](https://github.com/jdx) in [#872](#872) ### 🔍 Other Changes - add pr-closer workflow by [@jdx](https://github.com/jdx) in [#876](#876) ### 📦️ Dependency Updates - bump communique 1.0.3 → 1.0.4 by [@jdx](https://github.com/jdx) in [#868](#868) - update anthropics/claude-code-action digest to 2da6cfa by [@renovate[bot]](https://github.com/renovate[bot]) in [#869](#869) - update anthropics/claude-code-action digest to 567fe95 by [@renovate[bot]](https://github.com/renovate[bot]) in [#870](#870) - bump communique to 1.1.2 by [@jdx](https://github.com/jdx) in [#875](#875) <!-- CURSOR_SUMMARY --> --- > [!NOTE] > <sup>[Cursor Bugbot](https://cursor.com/bugbot) is generating a summary for commit 62ff432. Configure [here](https://www.cursor.com/dashboard/bugbot).</sup> <!-- /CURSOR_SUMMARY --> Co-authored-by: mise-en-dev <[email protected]>
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
1 participant
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Summary
HK_PKL_BACKEND=pklr, every load ofBuiltins.pklprinted[pklr] WARNING: property 'check_byte_order_marker' is deprecatedand the same forfix_byte_order_marker, even when the user'shk.pkldid not reference those aliases.Two coordinated changes
pklr0.4.1 → 0.4.2 (jdx/pklr#58, jdx/pklr#61). pklr now treats@Deprecatedlazily — the warning fires on field access, not on module evaluation.scripts/gen_builtins.py: special-case the deprecated aliases. Skip them in the auto-loop and emit them as@Deprecatedbindings that point at the canonicalbyte_order_markerstep. With this,Builtins.pkl's own bindings no longer access a deprecated property at load, so even the lazy field-access warning stays silent until the user explicitly referencesBuiltins.check_byte_order_marker/.fix_byte_order_marker.The deprecated stub files (
pkl/builtins/check_byte_order_marker.pkl,pkl/builtins/fix_byte_order_marker.pkl) keep their@Deprecatedannotations untouched — the migration nudge is preserved for direct importers.Test plan
mise run test:cargo— 145 passed.HK_PKL_BACKEND=pklr:Builtins.pklwithout using deprecated aliases → silent (was 2× warnings per load).Builtins.check_byte_order_marker→ warning fires once per evaluation.hkpre-commit hooks pass.🤖 Generated with Claude Code
Note
Low Risk
Low risk: changes are limited to a dependency patch bump and generation/mapping of builtin aliases to reduce deprecation warnings, with no impact on core hook execution logic beyond alias resolution.
Overview
Stops
Builtins.pklfrom triggering deprecation warnings on load when using thepklrbackend.This bumps
pklrfrom0.4.1to0.4.2and updatesscripts/gen_builtins.pyto skip generating deprecated builtin bindings in the main loop, then re-add them as explicit@Deprecatedaliases pointing to the canonicalbyte_order_markerbuiltin.The pre-commit migration map is updated so
check-byte-order-markerandfix-byte-order-markernow resolve directly toBuiltins.byte_order_markerinstead of the deprecated alias names.Reviewed by Cursor Bugbot for commit 549c070. Bugbot is set up for automated code reviews on this repo. Configure here.