Skip to content

Conversation

@youknowone
Copy link
Member

@youknowone youknowone commented Nov 14, 2025

fix #6254

Summary by CodeRabbit

  • Refactor
    • Updated system namespace implementation to compute values dynamically instead of using static literals.

@youknowone youknowone marked this pull request as ready for review November 14, 2025 14:11
@coderabbitai
Copy link
Contributor

coderabbitai bot commented Nov 14, 2025

Walkthrough

The sys::implementation function now dynamically computes the cache_tag field by combining a constant implementation name with the major and minor version numbers, instead of using a hardcoded string literal. Other namespace fields remain unchanged.

Changes

Cohort / File(s) Summary
sys::implementation cache_tag computation
vm/src/stdlib/sys.rs
Replaced hardcoded cache_tag value with dynamic computation using {NAME}-{MAJOR}{MINOR} format; name field now references a NAME constant instead of a literal string

Estimated code review effort

🎯 1 (Trivial) | ⏱️ ~5 minutes

  • Single file modification with straightforward logic replacement
  • String formatting using existing version constants
  • No architectural or control-flow changes

Poem

🐰 A cache tag once frozen in time,
"01" it was, oh what a crime!
Now swift like a hare, it bounds,
With versions that match—313 sounds!
Dynamic at last, the rabbit declares with delight! 🥕

Pre-merge checks and finishing touches

✅ Passed checks (5 passed)
Check name Status Explanation
Description Check ✅ Passed Check skipped - CodeRabbit’s high-level summary is enabled.
Title check ✅ Passed The title 'implement sys.implementation cache_tag' clearly and specifically summarizes the main change: implementing the cache_tag field in sys.implementation.
Linked Issues check ✅ Passed The PR fixes issue #6254 by replacing the hardcoded 'rustpython-01' cache_tag with dynamic computation using version::MAJOR and version::MINOR ('{NAME}-MAJORMINOR'), which now correctly returns 'rustpython-313'.
Out of Scope Changes check ✅ Passed All changes are directly scoped to fixing the cache_tag issue: only sys::implementation was modified to compute cache_tag dynamically. No extraneous alterations or unrelated changes detected.
Docstring Coverage ✅ Passed Docstring coverage is 100.00% which is sufficient. The required threshold is 80.00%.
✨ Finishing touches
  • 📝 Generate docstrings
🧪 Generate unit tests (beta)
  • Create PR with unit tests
  • Post copyable unit tests in a comment

📜 Recent review details

Configuration used: Path: .coderabbit.yml

Review profile: CHILL

Plan: Pro

📥 Commits

Reviewing files that changed from the base of the PR and between 9ce8586 and fb8d399.

📒 Files selected for processing (1)
  • vm/src/stdlib/sys.rs (1 hunks)
🧰 Additional context used
🧠 Learnings (2)
📚 Learning: 2025-06-27T14:47:28.810Z
Learnt from: moreal
Repo: RustPython/RustPython PR: 5847
File: vm/src/stdlib/stat.rs:547-567
Timestamp: 2025-06-27T14:47:28.810Z
Learning: In RustPython's stat module implementation, platform-specific constants like SF_SUPPORTED and SF_SYNTHETIC should be conditionally declared only for the platforms where they're available (e.g., macOS), following CPython's approach of optional declaration using #ifdef checks rather than providing fallback values for other platforms.

Applied to files:

  • vm/src/stdlib/sys.rs
📚 Learning: 2025-06-27T14:47:28.810Z
Learnt from: moreal
Repo: RustPython/RustPython PR: 5847
File: vm/src/stdlib/stat.rs:547-567
Timestamp: 2025-06-27T14:47:28.810Z
Learning: In RustPython's stat module implementation, platform-specific constants like SF_SUPPORTED and SF_SYNTHETIC should be conditionally declared only for the platforms where they're available (e.g., macOS), following CPython's approach of optional declaration rather than providing fallback values for other platforms.

Applied to files:

  • vm/src/stdlib/sys.rs
🧬 Code graph analysis (1)
vm/src/stdlib/sys.rs (1)
vm/src/vm/context.rs (1)
  • new_str (421-423)
⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (12)
  • GitHub Check: Run tests under miri
  • GitHub Check: Run snippets and cpython tests on wasm-wasi
  • GitHub Check: Ensure compilation on various targets
  • GitHub Check: Check the WASM package and demo
  • GitHub Check: Run snippets and cpython tests (windows-latest)
  • GitHub Check: Run snippets and cpython tests (ubuntu-latest)
  • GitHub Check: Run snippets and cpython tests (macos-latest)
  • GitHub Check: Check Rust code with clippy
  • GitHub Check: Run rust tests (ubuntu-latest)
  • GitHub Check: Run rust tests (macos-latest)
  • GitHub Check: Run rust tests (windows-latest)
  • GitHub Check: auto_format
🔇 Additional comments (2)
vm/src/stdlib/sys.rs (2)

237-238: Consistent use of the new NAME constant and computed cache_tag.

The namespace correctly uses the NAME constant and dynamically computed cache_tag, ensuring consistency throughout the implementation.


232-234: Good fix for the cache_tag computation.

The dynamic cache_tag computation correctly addresses the issue. The format produces "rustpython-313" matching PEP 3147 specifications. Verification confirms version::MAJOR and version::MINOR are correctly set to 3 and 13 respectively.

Since implementation() is decorated with #[pyattr], it's invoked once during module initialization and the result is cached as an attribute. The suggested Lazy<String> optimization is valid but has minimal practical impact given the single initialization call. The fix is correct as-is.


Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out.

❤️ Share

Comment @coderabbitai help to get the list of available commands and usage tips.

@youknowone youknowone merged commit 609d99f into RustPython:main Nov 14, 2025
13 checks passed
@youknowone youknowone deleted the cache_tag branch November 14, 2025 14:55
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

sys.implementation doesn't return proper cache_tag

1 participant