Reduce the number of EntryReader constructors
#606
Conversation
📝 WalkthroughWalkthroughAdded a new public 6-argument constructor to EntryReader that includes a boolean Possibly related PRs
Pre-merge checks and finishing touches❌ Failed checks (1 warning)
✨ Finishing touches
📜 Recent review detailsConfiguration used: Path: .coderabbit.yaml Review profile: ASSERTIVE Plan: Pro 📒 Files selected for processing (2)
⏰ 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). (4)
🔇 Additional comments (7)
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 |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 3
Fix all issues with AI Agents 🤖
In @CHANGELOG.md:
- Around line 10-14: Update the changelog entry under "## 1.13.0 (2026-??-??)"
by fixing the typo in the bullet about EntryReader: replace "unweildy" with the
correct spelling "unwieldy" so the line reads "There were an unwieldy number of
them."
In @src/main/java/org/plumelib/util/EntryReader.java:
- Line 406: The Javadoc @see for the constructor is incorrect: replace "Stream"
with "InputStream" in the @see tag that references EntryReader's constructor
signature (currently written as
#EntryReader(Stream,String,String,boolean,String,String)) so it correctly
references #EntryReader(InputStream,String,String,boolean,String,String).
- Around line 407-412: Update the @deprecated javadoc on EntryReader(Path,
String) to point to the InputStream-based constructor that preserves charset
support (the constructor invoked by this(...) that takes an InputStream and a
charset, e.g. EntryReader(InputStream, String, String, boolean, ...)), so users
retain charset-capable guidance; make the same change for the File- and
String-filename constructors that accept a charset (the other two deprecated
constructors noted) to reference their corresponding InputStream-based
charset-preserving constructor.
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (2)
src/main/java/org/plumelib/util/EntryReader.java (2)
223-238: Clarify charset requirement in deprecation message.The deprecation points to the 6-argument constructor that requires an explicit charset parameter, but this deprecated constructor implicitly uses UTF-8 (line 233). Users may be confused about which charset to pass to maintain equivalent behavior.
Consider updating the deprecation message to explicitly mention UTF-8:
🔎 Suggested deprecation message improvement
/** * Create an EntryReader. * * @param in source from which to read entries * @param twoBlankLines true if entries are separated by two blank lines rather than one * @param filename non-null file name for stream being read * @param commentRegexString regular expression that matches comments. Any text that matches * commentRegex is removed. A line that is entirely a comment is ignored. * @param includeRegexString regular expression that matches include directives. The expression * should define one group that contains the include file name. - * @deprecated use {@link #EntryReader(InputStream,String,String,boolean,String,String)} + * @deprecated use {@link #EntryReader(InputStream,String,String,boolean,String,String)} with "UTF-8" as the charset */Apply similar changes to the deprecation message at line 249.
513-522: Deprecation should point to String-based constructor, not File-based.The deprecation message recommends using
EntryReader(File,boolean,String,String), but there's a String-based equivalent at line 494:EntryReader(String filename, boolean twoBlankLines, String commentRegex, String includeRegex). Users working with String filenames would prefer to continue using the String-based API.🔎 Suggested fix
/** * Create a new EntryReader starting with the specified file. * * @param filename initial file to read * @param commentRegex regular expression that matches comments. Any text that matches {@code * commentRegex} is removed. A line that is entirely a comment is ignored. * @param includeRegex regular expression that matches include directives. The expression should * define one group that contains the include file name. * @throws IOException if there is a problem reading the file * @see #EntryReader(File,boolean,String,String) - * @deprecated use {@link #EntryReader(File,boolean,String,String)} + * @deprecated use {@link #EntryReader(String,boolean,String,String)} */
📜 Review details
Configuration used: Path: .coderabbit.yaml
Review profile: ASSERTIVE
Plan: Pro
📒 Files selected for processing (3)
CHANGELOG.mdsrc/main/java/org/plumelib/util/EntryReader.javasrc/test/java/org/plumelib/util/EntryReaderTest.java
💤 Files with no reviewable changes (1)
- src/test/java/org/plumelib/util/EntryReaderTest.java
🧰 Additional context used
🪛 LanguageTool
CHANGELOG.md
[grammar] ~13-~13: Ensure spelling is correct
Context: ...ted several constructors; there were an unweildy number of them. ## 1.12.3 (2025-11-26)...
(QB_NEW_EN_ORTHOGRAPHY_ERROR_IDS_1)
⏰ 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). (4)
- GitHub Check: build (25)
- GitHub Check: build (21)
- GitHub Check: build (17)
- GitHub Check: semgrep-cloud-platform/scan
🔇 Additional comments (2)
src/main/java/org/plumelib/util/EntryReader.java (2)
138-167: LGTM! Well-designed canonical constructor.The new 6-argument constructor provides a complete API surface and properly delegates to the Reader-based constructor. The design choice to make this the canonical constructor is sound.
1124-1190: LGTM! DummyReader properly reintroduced.The DummyReader class serves as a valid placeholder for the LineNumberReader superclass constructor (line 302), with all methods appropriately throwing errors except
close(), which intentionally doesn't throw to support try-with-resources patterns.
1 participant
No description provided.