refactor(#4729): move pinfos method to new PackageInfos class#4797
Conversation
…ackageInfos class
📝 WalkthroughWalkthroughThis PR refactors the package-info generation logic from Changes
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~22 minutes Possibly related PRs
Suggested labels
Suggested reviewers
Poem
🚥 Pre-merge checks | ✅ 4 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (4 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing touches
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 |
🚀 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: |
![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/PackageInfos.java:
- Around line 48-50: The Files.walk(this.root) stream in PackageInfos is not
closed; change the code that builds dirs to open the Stream<Path> in a
try-with-resources (e.g., try (Stream<Path> walk = Files.walk(this.root)) { dirs
= walk.filter(...).collect(...) }) so the stream is automatically closed; update
the declaration of dirs (and any surrounding scope) so the collected List<Path>
is assigned inside the try block and available afterward.
🧹 Nitpick comments (1)
eo-maven-plugin/src/test/java/org/eolang/maven/PackageInfosTest.java (1)
21-55: Consider adding a test for generated content verification.The existing tests verify file creation count and existence, but don't verify the content of generated
package-info.javafiles. Consider adding a test that verifies the package declaration matches the directory structure (e.g.,package subdir.subsubdir;for the nested directory).
📜 Review details
Configuration used: defaults
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (4)
eo-maven-plugin/src/main/java/org/eolang/maven/MjTranspile.javaeo-maven-plugin/src/main/java/org/eolang/maven/PackageInfos.javaeo-maven-plugin/src/test/java/org/eolang/maven/MjTranspileTest.javaeo-maven-plugin/src/test/java/org/eolang/maven/PackageInfosTest.java
💤 Files with no reviewable changes (1)
- eo-maven-plugin/src/test/java/org/eolang/maven/MjTranspileTest.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: benchmark
- GitHub Check: qulice
- GitHub Check: mvn (macos-15, 23)
- GitHub Check: trufflehog
- GitHub Check: mvn (ubuntu-24.04, 23)
- GitHub Check: mvn (ubuntu-24.04, 17)
- GitHub Check: snippets
- GitHub Check: mvn (windows-2022, 23)
- GitHub Check: ort
- GitHub Check: pr-size
- GitHub Check: integration
🔇 Additional comments (5)
eo-maven-plugin/src/main/java/org/eolang/maven/PackageInfos.java (2)
70-83: LGTM!The content generation logic is clean and correctly produces the expected
package-info.javastructure with the auto-generated header and package declaration.
20-38: LGTM!The class structure follows good practices: final class with immutable state, package-private visibility for internal use, and clear Javadoc.
eo-maven-plugin/src/main/java/org/eolang/maven/MjTranspile.java (1)
118-118: LGTM!Clean delegation to the new
PackageInfosutility class. The refactoring maintains the same behavior while improving code organization by separating the package-info generation concern.eo-maven-plugin/src/test/java/org/eolang/maven/PackageInfosTest.java (2)
24-41: LGTM!Good test coverage for the happy path scenario with nested subdirectories. The assertions correctly verify both the count and file existence.
43-55: LGTM!Correctly tests the edge case where the root directory has no subdirectories, ensuring no package-info.java is created in the root itself.
| final List<Path> dirs = Files.walk(this.root) | ||
| .filter(file -> Files.isDirectory(file) && !file.equals(this.root)) | ||
| .collect(Collectors.toList()); |
There was a problem hiding this comment.
Resource leak: Files.walk() stream is not closed.
Files.walk() returns a Stream<Path> backed by I/O resources that must be explicitly closed. Using .collect() does not close the underlying stream, which can lead to file handle exhaustion.
🔧 Proposed fix using try-with-resources
if (Files.exists(this.root)) {
- final List<Path> dirs = Files.walk(this.root)
- .filter(file -> Files.isDirectory(file) && !file.equals(this.root))
- .collect(Collectors.toList());
+ final List<Path> dirs;
+ try (java.util.stream.Stream<Path> stream = Files.walk(this.root)) {
+ dirs = stream
+ .filter(file -> Files.isDirectory(file) && !file.equals(this.root))
+ .collect(Collectors.toList());
+ }
for (final Path dir : dirs) {📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| final List<Path> dirs = Files.walk(this.root) | |
| .filter(file -> Files.isDirectory(file) && !file.equals(this.root)) | |
| .collect(Collectors.toList()); | |
| final List<Path> dirs; | |
| try (java.util.stream.Stream<Path> stream = Files.walk(this.root)) { | |
| dirs = stream | |
| .filter(file -> Files.isDirectory(file) && !file.equals(this.root)) | |
| .collect(Collectors.toList()); | |
| } |
🤖 Prompt for AI Agents
In @eo-maven-plugin/src/main/java/org/eolang/maven/PackageInfos.java around
lines 48 - 50, The Files.walk(this.root) stream in PackageInfos is not closed;
change the code that builds dirs to open the Stream<Path> in a
try-with-resources (e.g., try (Stream<Path> walk = Files.walk(this.root)) { dirs
= walk.filter(...).collect(...) }) so the stream is automatically closed; update
the declaration of dirs (and any surrounding scope) so the collected List<Path>
is assigned inside the try block and available afterward.
There was a problem hiding this comment.
Pull request overview
This PR refactors the pinfos(Path) method from MjTranspile.java into a new standalone class PackageInfos, improving adherence to the Single Responsibility Principle. The refactoring resolves puzzle #4717.
Changes:
- Created new
PackageInfosclass to encapsulate package-info.java file creation logic - Added comprehensive unit tests for the new
PackageInfosclass inPackageInfosTest.java - Updated
MjTranspile.javato use the new class and removed unused imports - Enabled previously disabled test
savesValidContentToPackageInfoFileinMjTranspileTest.java
Reviewed changes
Copilot reviewed 4 out of 4 changed files in this pull request and generated 4 comments.
| File | Description |
|---|---|
| PackageInfos.java | New class containing refactored logic for creating package-info.java files in subdirectories |
| PackageInfosTest.java | New test file with comprehensive unit tests for PackageInfos class |
| MjTranspile.java | Removed pinfos method and updated to use new PackageInfos class; cleaned up unused imports |
| MjTranspileTest.java | Re-enabled test that verifies valid content in package-info.java files |
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
| MatcherAssert.assertThat( | ||
| "We should create exactly two package-info.java files for two subdirectories", | ||
| new PackageInfos(tmp).create(), | ||
| Matchers.equalTo(2) | ||
| ); | ||
| MatcherAssert.assertThat( | ||
| "package-info.java should be created in the both subdirectories", | ||
| Files.exists(subdir.resolve("package-info.java")) | ||
| && Files.exists(subsubdir.resolve("package-info.java")), | ||
| Matchers.is(true) | ||
| ); |
There was a problem hiding this comment.
This test contains two assertions, which violates the project guideline that each test should contain only one assertion. Consider splitting this into two separate tests: one to verify the count of created files, and another to verify the existence of package-info.java files in subdirectories.
| MatcherAssert.assertThat( | ||
| "No package-info.java files should be created in the root directory", | ||
| new PackageInfos(tmp).create(), | ||
| Matchers.equalTo(0) | ||
| ); | ||
| MatcherAssert.assertThat( | ||
| "package-info.java should not be created in the root directory", | ||
| Files.exists(tmp.resolve("package-info.java")), | ||
| Matchers.is(false) | ||
| ); |
There was a problem hiding this comment.
This test contains two assertions, which violates the project guideline that each test should contain only one assertion. Consider splitting this into two separate tests: one to verify the count of created files is 0, and another to verify package-info.java does not exist in the root directory.
| final List<Path> dirs = Files.walk(this.root) | ||
| .filter(file -> Files.isDirectory(file) && !file.equals(this.root)) | ||
| .collect(Collectors.toList()); |
There was a problem hiding this comment.
The variable name "dirs" is a compound abbreviation. According to the project guidelines, compound variable names should be avoided in favor of single-word nouns. Consider renaming to "directories" or simply "paths" to follow the naming convention.
| final String pkg = this.root.relativize(dir).toString() | ||
| .replace(File.separator, "."); |
There was a problem hiding this comment.
The variable name "pkg" is an abbreviation. According to the project guidelines, compound or abbreviated variable names should be avoided in favor of single-word nouns. Consider renaming to "package" (though this is a Java keyword, so "name" might be a better alternative).
|
@yegor256 could you have a look, please? |
|
@volodya-lombrozo thanks! |
This PR refactors the
pinfos(Path)method fromMjTranspile.javainto a new classPackageInfos, resolving puzzle4717.Fixes #4729
Summary by CodeRabbit
Release Notes
Refactor
Tests
✏️ Tip: You can customize this high-level summary in your review settings.