fix(#4840): remove 'semver' field from 'FpIfRleased' and improve logging#4843
Conversation
📝 WalkthroughWalkthroughCache orchestration in the Maven plugin was simplified: semver parameters were removed from FpDefault and FpIfReleased, cache decisions now rely on hash presence, call sites updated, and MjTranspile logging gained file-modification info. Changes
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~22 minutes Possibly related PRs
Suggested labels
Suggested reviewers
Poem
🚥 Pre-merge checks | ✅ 3✅ 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 |
There was a problem hiding this comment.
Pull request overview
This PR removes the unused semver parameter from FpIfReleased and adds more detailed debug logging around XMIR transpilation and Java generation.
Changes:
- Removed the
semverargument fromFpIfReleasedconstructors and updated call sites/tests accordingly. - Added debug timing logs for cache-miss XMIR transpilation and Java file generation in
MjTranspile. - Enabled Maven debug output (
-X) for the Fibonacci integration test invoker run.
Reviewed changes
Copilot reviewed 6 out of 6 changed files in this pull request and generated 6 comments.
Show a summary per file
| File | Description |
|---|---|
| eo-maven-plugin/src/test/java/org/eolang/maven/FpIfReleasedTest.java | Updates tests to use the new FpIfReleased constructor signature (no semver). |
| eo-maven-plugin/src/main/java/org/eolang/maven/MjTranspile.java | Adds debug/timing logs for transpilation and Java generation; introduces info(Path) helper for log metadata. |
| eo-maven-plugin/src/main/java/org/eolang/maven/MjPull.java | Updates FpIfReleased construction to match the new signature. |
| eo-maven-plugin/src/main/java/org/eolang/maven/FpIfReleased.java | Removes semver from the API and adjusts logging condition to be hash-only. |
| eo-maven-plugin/src/main/java/org/eolang/maven/FpDefault.java | Removes outdated references to version-based caching behavior and updates wiring to new FpIfReleased signature. |
| eo-integration-tests/src/it/fibonacci/invoker.properties | Adds -X (Maven debug) to invoker goals for the Fibonacci integration test. |
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
| * @throws IOException If fails | ||
| */ | ||
| private static String info(final Path info) throws IOException { | ||
| final String res; | ||
| if (Files.exists(info)) { | ||
| res = Files.getLastModifiedTime(info).toString(); | ||
| } else { | ||
| res = "Not exists yet"; |
There was a problem hiding this comment.
info(Path) is used only to enrich debug logging, but it can throw IOException (e.g., from Files.getLastModifiedTime) and that exception will propagate via UncheckedFunc and can fail the transpilation even though the main work succeeded. Consider handling IOException inside info (returning a safe fallback string) and removing the checked throws IOException from this helper.
| * @throws IOException If fails | |
| */ | |
| private static String info(final Path info) throws IOException { | |
| final String res; | |
| if (Files.exists(info)) { | |
| res = Files.getLastModifiedTime(info).toString(); | |
| } else { | |
| res = "Not exists yet"; | |
| */ | |
| private static String info(final Path info) { | |
| final String res; | |
| try { | |
| if (Files.exists(info)) { | |
| res = Files.getLastModifiedTime(info).toString(); | |
| } else { | |
| res = "Not exists yet"; | |
| } | |
| } catch (final IOException ex) { | |
| res = "Unavailable"; |
| if (Files.exists(info)) { | ||
| res = Files.getLastModifiedTime(info).toString(); | ||
| } else { | ||
| res = "Not exists yet"; |
There was a problem hiding this comment.
Log message text Not exists yet is grammatically incorrect; consider changing it to something like Does not exist yet to keep debug output professional and clear.
| res = "Not exists yet"; | |
| res = "Does not exist yet"; |
| final String res = transform.apply(xmir).toString(); | ||
| Logger.debug( | ||
| this, | ||
| "Transpiled %[file]s (%s) to %[file]s (%s) in %[ms]s (cache miss)", | ||
| source, | ||
| MjTranspile.info(source), |
There was a problem hiding this comment.
The src -> { ... } lambda passed to FpDefault ignores its src parameter and instead uses the captured source variable. Using the lambda parameter would make the code clearer and avoids accidentally logging the wrong file if this code is refactored to reuse the function elsewhere.
| final String res = transform.apply(xmir).toString(); | |
| Logger.debug( | |
| this, | |
| "Transpiled %[file]s (%s) to %[file]s (%s) in %[ms]s (cache miss)", | |
| source, | |
| MjTranspile.info(source), | |
| final String res = transform.apply(new XMLDocument(src)).toString(); | |
| Logger.debug( | |
| this, | |
| "Transpiled %[file]s (%s) to %[file]s (%s) in %[ms]s (cache miss)", | |
| src, | |
| MjTranspile.info(src), |
| * Ctor. | ||
| * @param semver Cache version | ||
| * @param hash Git hash | ||
| * @param first First footprint to use if a version is released and a hash is present | ||
| * @param second Second footprint to use if a version is not released or a hash is not present |
There was a problem hiding this comment.
The constructor Javadoc still refers to “a version is released / not released”, but the class no longer receives or checks a version—only whether the hash is empty. Please update the first/second parameter descriptions to match the new behavior (hash-present vs hash-absent).
| /** | ||
| * Ctor. | ||
| * <p>Here {@link FpIfReleased} is on the first place because we don't want to work with any | ||
| * type of cache (local or global) if we work with SNAPSHOT version of the plugin</p> |
There was a problem hiding this comment.
This Javadoc claims FpIfReleased is used to avoid any caching for SNAPSHOT plugin versions, but FpIfReleased no longer checks the plugin version (only hash emptiness). Either update the documentation to reflect current behavior, or reintroduce the version-based condition if SNAPSHOT disabling is still required.
| * type of cache (local or global) if we work with SNAPSHOT version of the plugin</p> | |
| * type of cache (local or global) when the cache hash indicates that the version is not | |
| * released (for example, when the hash is empty).</p> |
| # SPDX-FileCopyrightText: Copyright (c) 2016-2026 Objectionary.com | ||
| # SPDX-License-Identifier: MIT | ||
| invoker.goals = clean test | ||
| invoker.goals = clean test -X |
There was a problem hiding this comment.
Adding Maven -X to integration test goals will significantly increase CI log volume and can slow runs. If this is only for local debugging, consider reverting it or gating it behind a profile/property so CI stays quiet by default.
9defbe1 to
f33ff81
Compare
🚀 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
|
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
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/MjTranspile.java`:
- Around line 183-194: Remove the unresolved merge markers (<<<<<<<, =======,
>>>>>>>) and restore a single coherent log call: use the expanded format string
with MjTranspile.info calls so the arguments match the format — e.g. "Transpiled
%[file]s (%s) to %[file]s (%s) in %[ms]s (cache miss)", source,
MjTranspile.info(source), target, MjTranspile.info(target),
System.currentTimeMillis() - start — and ensure this replaces the conflicted
block around the logging statement in MjTranspile.
🧹 Nitpick comments (1)
eo-maven-plugin/src/main/java/org/eolang/maven/MjTranspile.java (1)
207-221: Makeinfo(...)best‑effort to avoid logging failures
infois only used for logging; throwingIOExceptioncan fail transpilation when file metadata can’t be read. Consider catching and returning a fallback string.🔧 Suggested adjustment
- private static String info(final Path info) throws IOException { - final String res; - if (Files.exists(info)) { - res = Files.getLastModifiedTime(info).toString(); - } else { - res = "Not exists yet"; - } - return res; - } + private static String info(final Path info) { + try { + if (Files.exists(info)) { + return Files.getLastModifiedTime(info).toString(); + } + return "Not exists yet"; + } catch (final IOException err) { + return "Unavailable"; + } + }
| <<<<<<< HEAD | ||
| "Transpiled %[file]s to %[file]s in %[ms]s (cache miss)", | ||
| source, | ||
| target, | ||
| ======= | ||
| "Transpiled %[file]s (%s) to %[file]s (%s) in %[ms]s (cache miss)", | ||
| source, | ||
| MjTranspile.info(source), | ||
| target, | ||
| MjTranspile.info(target), | ||
| >>>>>>> 9defbe1d6 (refactor(#4840): Remove semver parameter from FpIfReleased and related classes) | ||
| System.currentTimeMillis() - start |
There was a problem hiding this comment.
Resolve merge conflict markers to restore compilation
Unresolved conflict markers around Line 183-194 are breaking the build (illegal start of expression). Please resolve by keeping the intended log message.
🛠️ Proposed fix
- Logger.debug(
- this,
-<<<<<<< HEAD
- "Transpiled %[file]s to %[file]s in %[ms]s (cache miss)",
- source,
- target,
-=======
- "Transpiled %[file]s (%s) to %[file]s (%s) in %[ms]s (cache miss)",
- source,
- MjTranspile.info(source),
- target,
- MjTranspile.info(target),
->>>>>>> 9defbe1d6 (refactor(`#4840`): Remove semver parameter from FpIfReleased and related classes)
- System.currentTimeMillis() - start
- );
+ Logger.debug(
+ this,
+ "Transpiled %[file]s (%s) to %[file]s (%s) in %[ms]s (cache miss)",
+ source,
+ MjTranspile.info(source),
+ target,
+ MjTranspile.info(target),
+ System.currentTimeMillis() - start
+ );🧰 Tools
🪛 GitHub Actions: integration
[error] 183-194: Compilation failure: illegal start of expression and related syntax errors in MjTranspile.java (lines 183-194) causing Maven to fail during 'mvn clean install -PskipUTs --errors --batch-mode'.
🪛 GitHub Actions: qulice
[error] 183-194: Compilation failed. Illegal start of expression in MjTranspile.java at lines 183-194. Followed by multiple syntax errors (e.g., illegal start of expression, not a statement, missing semicolons, and unexpected characters). This indicates a Java syntax/structural issue in the transpile logic causing Maven compile to fail.
🪛 GitHub Actions: snippets
[error] 183-194: Compilation failure in eo-maven-plugin: illegal start of expression and multiple syntax errors in MjTranspile.java. Likely caused by malformed code near line 183 (e.g., missing braces or incorrect syntax). See Maven output for details.
🤖 Prompt for AI Agents
In `@eo-maven-plugin/src/main/java/org/eolang/maven/MjTranspile.java` around lines
183 - 194, Remove the unresolved merge markers (<<<<<<<, =======, >>>>>>>) and
restore a single coherent log call: use the expanded format string with
MjTranspile.info calls so the arguments match the format — e.g. "Transpiled
%[file]s (%s) to %[file]s (%s) in %[ms]s (cache miss)", source,
MjTranspile.info(source), target, MjTranspile.info(target),
System.currentTimeMillis() - start — and ensure this replaces the conflicted
block around the logging statement in MjTranspile.
…d and related classes
f33ff81 to
88e5192
Compare
There was a problem hiding this comment.
Actionable comments posted: 0
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
eo-maven-plugin/src/main/java/org/eolang/maven/FpIfReleased.java (1)
46-63:⚠️ Potential issue | 🟠 MajorGuard against null hashes to avoid NPEs.
If the supplier returnsnull,isEmpty()will throw and break the build. Consider a null-safe check.🔧 Suggested fix
- final String hsh = hash.get(); - final boolean cacheable = !hsh.isEmpty(); + final String hsh = hash.get(); + final boolean cacheable = hsh != null && !hsh.isEmpty();
|
@yegor256 can you take a look, please? |
|
@volodya-lombrozo Hey there! 🎉 Nice work on your contribution - you've snagged +16 points for this one! The policy rewards solid contributions like yours, and you're clearly hitting the sweet spot with good code quality and proper review process. Your running score is looking great at +248, so keep those quality contributions coming! Don't forget to check your Zerocracy account for the latest updates. |
In this PR I slightly improved logging and what is the most important, removed unused 'semver' field from 'FpIfReleased' class.
Related to #4840
Summary by CodeRabbit
Refactor
Tests
✏️ Tip: You can customize this high-level summary in your review settings.