refactor(#4851): replace FpDefault with Cache in caching logic#4861
Merged
yegor256 merged 2 commits intoFeb 6, 2026
Conversation
Contributor
📝 WalkthroughWalkthroughReplaces inlined FpDefault usage with explicit Cache/CachePath constructors and branches mojo flows on Changes
Sequence Diagram(s)sequenceDiagram
participant Mojo as "Mojo (MjParse / MjLint / MjTranspile)"
participant CCache as "ConcurrentCache"
participant Cache as "Cache / CachePath"
participant FS as "Filesystem (target, hash files)"
Mojo->>Mojo: determine cacheEnabled
alt cacheEnabled == true
Mojo->>CCache: request apply(key, source, target, tail)
CCache->>Cache: lookup / apply(source, target, tail)
Cache->>FS: read hash file / write hash & write target XMIR
FS-->>Cache: confirmation
Cache-->>CCache: result (target path)
CCache-->>Mojo: returns target path
else cacheEnabled == false
Mojo->>Mojo: compute result directly (parsed / linted / transpiled)
Mojo->>FS: save target XMIR
FS-->>Mojo: confirmation
end
Mojo->>Mojo: update tojo metadata (withXmir / withLinted / version)
Estimated code review effort🎯 4 (Complex) | ⏱️ ~45 minutes Possibly related PRs
Suggested labels
Suggested reviewers
Poem
🚥 Pre-merge checks | ✅ 3 | ❌ 2❌ Failed checks (2 warnings)
✅ Passed checks (3 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing touches
🧪 Generate unit tests (beta)
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. Comment |
Contributor
🚀 Performance AnalysisAll benchmarks are within the acceptable range. No critical degradation detected (threshold is 100%). Please refer to the detailed report for more information. Click to see the detailed report
✅ Performance gain: |
Contributor
There was a problem hiding this comment.
Pull request overview
This PR refactors the caching logic to replace FpDefault with the newer Cache class implementation, addressing puzzle #4846. The changes update three main Maven mojos (MjParse, MjLint, and MjTranspile) to use the simplified Cache API, add convenience constructors to support the new usage patterns, and disable some tests that require repair due to the new caching approach.
Changes:
- Replaced
FpDefaultusage withCacheandConcurrentCachein MjParse, MjLint, and MjTranspile - Added new constructors to
CacheandCachePathfor convenience - Disabled three cache-related tests in MjLintTest with TODO for future repair
- Removed puzzle comment from Cache.java as it has been resolved
Reviewed changes
Copilot reviewed 8 out of 8 changed files in this pull request and generated 6 comments.
Show a summary per file
| File | Description |
|---|---|
| Cache.java | Removed resolved puzzle comment, added constructor accepting CachePath, refactored hash method formatting |
| CachePath.java | Added convenience constructor for creating cache paths without explicit tail parameter |
| FpDefault.java | Added TODO comment indicating future removal since it's now only used in tests |
| MjParse.java | Replaced FpDefault with Cache/ConcurrentCache using CachePath for proper version/hash incorporation |
| MjLint.java | Replaced FpDefault with Cache/ConcurrentCache and added conditional logic for cache enabled/disabled paths |
| MjTranspile.java | Replaced FpDefault with Cache/ConcurrentCache and added conditional logic for cache enabled/disabled paths |
| MjParseTest.java | Updated test to use new Cache API directly, removed unused Paths import and obsolete cache timestamp manipulation |
| MjLintTest.java | Disabled three cache-related tests with TODO explaining they need repair after caching logic changes |
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
Contributor
There was a problem hiding this comment.
Actionable comments posted: 4
🤖 Fix all issues with AI agents
In `@eo-maven-plugin/src/main/java/org/eolang/maven/Cache.java`:
- Around line 90-94: The hash(Path tail) method can NPE because full.getParent()
may be null; update hash to defensively handle parent==null by computing the sha
filename as before (String.format("%s.sha256", full.getFileName().toString()))
and then use parent.resolve(format) when parent != null, otherwise resolve the
format against this.base (or use full.resolveSibling as an alternative) so the
method never calls resolve on a null parent; change references to full, parent,
and the returned Path accordingly in the hash(...) method.
In `@eo-maven-plugin/src/main/java/org/eolang/maven/MjLint.java`:
- Around line 155-175: The else branch calls linted(...) but discards its
returned content so the linted XML is never written to target; change the else
path to obtain the result of this.linted(tojo.identifier(), xmir, counts,
unlints) and write it to the same target file (the same behavior as the cache
branch), then call tojo.withLinted(target). Locate the linting invocation in
MjLint where ConcurrentCache/Cache are used and update the non-cached branch to
persist the linted output produced by the linted(...) method before calling
tojo.withLinted(target).
In `@eo-maven-plugin/src/main/java/org/eolang/maven/MjParse.java`:
- Around line 107-125: When cacheEnabled is false the parsed XMIR isn't
persisted: change the else branch in MjParse to capture Node node =
this.parsed(source, name); refs.add(node); then write the serialized XMIR (new
XMLDocument(node).toString()) to the target path (creating parent directories as
needed) before calling tojo.withXmir(target). Ensure you mirror the cache
branch's output format so MjParse.version(target, refs) and
tojo.withXmir(target) operate on an actual file.
In `@eo-maven-plugin/src/main/java/org/eolang/maven/MjTranspile.java`:
- Around line 179-208: The else branch in MjTranspile currently calls
transform.apply(xmir) but discards its result so the generated Java file at
target is never written when cacheEnabled is false; change the else branch to
mirror the cache-miss behavior by capturing the transformed result (e.g., String
res = transform.apply(xmir).toString()), write that result to the target file
(using the same file-write approach used elsewhere), and update
rewrite.compareAndSet(false, true) and any logging similarly so
Files.exists(target) will be true for subsequent javaGenerated checks.
🧹 Nitpick comments (1)
eo-maven-plugin/src/test/java/org/eolang/maven/MjLintTest.java (1)
173-175: Add reason strings to all three@Disabledannotations for traceability. JUnit 5 supports the@Disabledannotation with a reason string parameter to document why tests are skipped. The class javadoc already references issue#4851and explains the reason (caching logic changes), so add corresponding reason strings to each disabled test:
skipsAlreadyLinted(line 174)savesVerifiedResultsToCache(line 193)getsAlreadyVerifiedResultsFromCacheExample:
Apply to all three disabled tests
- `@Disabled` + `@Disabled`("TODO `#4851`: repair caching tests after Cache refactor")
Contributor
There was a problem hiding this comment.
Actionable comments posted: 1
🤖 Fix all issues with AI agents
In `@eo-maven-plugin/src/main/java/org/eolang/maven/MjLint.java`:
- Around line 155-182: The cache-hit branch in MjLint (ConcurrentCache/Cache
usage) doesn't execute the provided lambda so linted(...) is not called and the
defect counters (counts, unlints) are not updated; after applying the cache (or
after Saved.value() in the non-cache branch) call linted(tojo.identifier(),
xmir, counts, unlints) or alternatively parse the resulting target XMIR's
<errors> elements to increment counts and unlints so the summary and the
ERROR/CRITICAL build-failure check reflect cached defects; ensure this happens
before tojo.withLinted(target) so the counters are accurate for subsequent
checks.
volodya-lombrozo
force-pushed
the
4851-replace-fpdefault
branch
from
c866355 to
dc239f9
Compare
…esults in MjLint, MjParse, and MjTranspile
volodya-lombrozo
force-pushed
the
4851-replace-fpdefault
branch
from
dc239f9 to
02402a8
Compare
Member
Author
|
@yegor256, could you check this one, please? |
|
@volodya-lombrozo Thanks for the contribution! You've earned +12 points for this: +16 as a basis, +10 for hits-of-code (200 * 0.05), -4 for more than 100 hits-of-code (200 >= 100), and -8 for more than 400 hits-of-code. Please, keep them coming. Your running score is +367; don't forget to check your Zerocracy account too. |
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
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.
This PR replaces the use of
FpDefault->Cacheresolving puzzle4846.Fixes #4851
Summary by CodeRabbit
Refactor
Documentation
Tests