fix(#4730): better message for missing 'package-info.class' in PhPackage.java#4784
Conversation
|
Note Other AI code review bot(s) detectedCodeRabbit has detected other AI code review bot(s) in this pull request and will avoid duplicating their findings in the review comments. This may lead to a less comprehensive review. 📝 WalkthroughWalkthroughThe changes improve package-info class resolution error handling in the PhPackage runtime. A dedicated package-info class path lookup is introduced, providing explicit error messages when the package-info class cannot be found, with fallback to the target class. Corresponding test coverage and integration test assertions are updated to validate the new behavior. Changes
Estimated code review effort🎯 2 (Simple) | ⏱️ ~12 minutes Suggested labels
Poem
Pre-merge checks and finishing touches❌ Failed checks (1 warning)
✅ Passed checks (4 passed)
✨ Finishing touches
📜 Recent review detailsConfiguration used: defaults Review profile: CHILL Plan: Pro 📒 Files selected for processing (3)
🧰 Additional context used🧠 Learnings (2)📚 Learning: 2025-08-12T15:24:05.011ZApplied to files:
📚 Learning: 2025-10-17T14:21:45.092ZApplied to files:
⏰ 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)
🔇 Additional comments (4)
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 |
|
Blocked by #4782 |
🚀 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
|
There was a problem hiding this comment.
Pull request overview
This PR improves the error message when a package-info.class file is missing during object lookup in the EO runtime. The error message now explicitly mentions both the missing class and the missing package-info class, making it clearer what went wrong. Additionally, the PR includes a refactoring cleanup that removes a duplicate test utility class.
- Enhanced error message in
PhPackage.javato explicitly mention the missing package-info class - Added test coverage to verify the improved error message
- Refactored test utilities to use standard matchers instead of custom duplicate classes
Reviewed changes
Copilot reviewed 5 out of 5 changed files in this pull request and generated 2 comments.
Show a summary per file
| File | Description |
|---|---|
| eo-runtime/src/main/java/org/eolang/PhPackage.java | Improved error message format to explicitly mention missing package-info class and removed completed TODO |
| eo-runtime/src/test/java/org/eolang/PhPackageTest.java | Added test to verify the improved error message for missing package-info class |
| eo-integration-tests/src/test/java/org/eolang/maven/MjAssembleIT.java | Renamed method for clarity and replaced custom matcher with standard FileMatchers |
| eo-integration-tests/src/test/java/org/eolang/maven/ContainsFiles.java | Removed duplicate utility class that is no longer needed |
| .github/workflows/simian.yml | Removed exclusion for deleted ContainsFiles.java |
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
| private Phi loadPhi(final String fqn) { | ||
| final String target = new JavaPath(fqn).toString(); | ||
| final String pinfo = String.format("%s.package-info", target); | ||
| Phi loaded; |
There was a problem hiding this comment.
According to the coding guidelines, blank lines should not be used inside method bodies as they reduce code readability. Remove this blank line.
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Nitpick comments (1)
eo-runtime/src/test/java/org/eolang/PhPackageTest.java (1)
113-126: Good test coverage for missing package-info!The test correctly verifies that the error message now explicitly mentions both the missing class and package-info class, which improves debuggability.
Optional: Fix method name consistency
The method name uses plural "Exceptions" but only one exception is thrown. For consistency with line 105 (
throwsExceptionIfCantInstantiateObject), consider:- void throwsExceptionsIfCantFindPackageInfo() { + void throwsExceptionIfCantFindPackageInfo() {
📜 Review details
Configuration used: defaults
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (5)
.github/workflows/simian.ymleo-integration-tests/src/test/java/org/eolang/maven/ContainsFiles.javaeo-integration-tests/src/test/java/org/eolang/maven/MjAssembleIT.javaeo-runtime/src/main/java/org/eolang/PhPackage.javaeo-runtime/src/test/java/org/eolang/PhPackageTest.java
💤 Files with no reviewable changes (1)
- eo-integration-tests/src/test/java/org/eolang/maven/ContainsFiles.java
🧰 Additional context used
🧠 Learnings (1)
📚 Learning: 2025-08-12T15:24:05.011Z
Learnt from: h1alexbel
Repo: objectionary/eo PR: 4431
File: eo-parser/src/main/resources/XMIR.xsd:80-81
Timestamp: 2025-08-12T15:24:05.011Z
Learning: In the EO parser architecture, Φ̇ is an alias for Φ.org.eolang that gets expanded during XMIR decoration by expand-qqs.xsl. The XSD schema for FQN patterns should only validate canonical forms (Φ and ξ as roots) since alias expansion happens before XSD validation.
Applied to files:
eo-runtime/src/main/java/org/eolang/PhPackage.java
⏰ 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). (14)
- GitHub Check: CodeQL analysis (java)
- GitHub Check: Agent
- GitHub Check: Codacy Static Code Analysis
- GitHub Check: qulice
- GitHub Check: benchmark
- GitHub Check: mvn (macos-15, 23)
- GitHub Check: integration
- GitHub Check: ort
- GitHub Check: snippets
- GitHub Check: mvn (ubuntu-24.04, 23)
- GitHub Check: mvn (windows-2022, 23)
- GitHub Check: trufflehog
- GitHub Check: mvn (ubuntu-24.04, 17)
- GitHub Check: pr-size
🔇 Additional comments (5)
.github/workflows/simian.yml (1)
24-24: LGTM!The exclusion is correctly removed since
ContainsFiles.javano longer exists after being deleted in this PR.eo-integration-tests/src/test/java/org/eolang/maven/MjAssembleIT.java (3)
18-18: LGTM!The addition of
FileMatchersimport aligns with replacing the customContainsFilesmatcher with standard Hamcrest matchers.
42-42: LGTM on the method rename!The rename from
appendItselftoregisterAssembleis clearer and better describes what the method does—registering and assembling the plugin.Also applies to: 83-83, 107-110
87-89: LGTM on using FileMatchers!Using
FileMatchers.anExistingFile()with a concrete path is more idiomatic and maintainable than a custom matcher.eo-runtime/src/main/java/org/eolang/PhPackage.java (1)
129-143: Excellent improvement to error messaging!The change successfully addresses issue #4730 by explicitly checking for
package-info.classexistence and providing a clear error message when neither the class nor package-info is found. The error message now tells users exactly what's missing, which significantly improves debuggability.The logic correctly:
- Constructs the package-info path
- Attempts to load it first to determine if it's a package
- Falls back to loading the target class if package-info doesn't exist
- Reports both expected paths in the error message when both are missing
volodya-lombrozo
force-pushed
the
4730-ph-package-error
branch
from
99fca89 to
b5ee9e4
Compare
volodya-lombrozo
force-pushed
the
4730-ph-package-error
branch
from
b5ee9e4 to
49084f5
Compare
|
@yegor256 could you have a look, please? |
Co-authored-by: Copilot <175728472+Copilot@users.noreply.github.com>
This PR improves the verification message of a missing
package-info.classinPhPackage.java.Fixes #4730
Summary by CodeRabbit
Bug Fixes
Tests
✏️ Tip: You can customize this high-level summary in your review settings.