Code review#490
Merged
Merged
Conversation
📝 WalkthroughWalkthroughThis pull request makes three unrelated cleanup changes: it updates the "iCalendar format" documentation link in README.md to use https, removes a duplicated JVM option from the java invocation in the ical-available elisp script, and renames several identifiers in ICalAvailable.java (remaining_args to remainingArgs, URL loop variable to iCalUrl, date-format loop variable to dateFormat), while also adding a debug branch that streams calendar content bytes to standard output when a specific ParserException occurs. Changes
Sequence Diagram(s)sequenceDiagram
participant CalendarLoadingLoop
participant ParserExceptionHandler
participant contentStream
participant StandardOutput
CalendarLoadingLoop->>ParserExceptionHandler: handle ParserException
ParserExceptionHandler->>contentStream: open try-with-resources stream
ParserExceptionHandler->>StandardOutput: print URL and "Contents:"
contentStream->>StandardOutput: copy response bytes in 1024-byte chunks
CalendarLoadingLoop->>StandardOutput: print failure message with iCalUrl
Estimated code review effort: Low Related issues: None found in provided context. Related PRs: None found in provided context. Suggested labels: cleanup, documentation, refactor Suggested reviewers: None found in provided context. 🐰 A tweak here, a rename there, <visible_sequence_diagrams> Sequence Diagram(s)sequenceDiagram
participant ComponentA
participant ComponentB
ComponentA->>ComponentB: observable interaction
</visible_sequence_diagrams> 🚥 Pre-merge checks | ✅ 3✅ Passed checks (3 passed)
✨ 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 |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
src/main/java/org/plumelib/icalavailable/ICalAvailable.java (1)
235-247: 🎯 Functional Correctness | 🟠 Major | ⚡ Quick winDebug output not gated by
--debugoption, and refetches the URL.Two concerns with this new block:
- It always executes when the specific
ParserExceptionmessage matches, regardless of thedebugflag. Every other diagnostic block in this file (e.g. lines 386-388, 496-498, 533-539, 542-544, 551-553, 556-558) is gated withif (debug) { ... }. Here, unconditionally dumping the entire remote resource to stdout on this error path is inconsistent with that convention and will pollute output for all users hitting this parse error, not just those who opted into--debug.url.openStream()is called a second time to fetch content for printing, after the first stream (urlStream) already read (and failed to parse) the body. If the resource is dynamic, the printed content may not match what actually caused theParserException, making the debug output misleading. Consider buffering the bytes from the originalurlStreambefore parsing so the same content used for the failed parse is available for debug printing, avoiding the extra network round trip.💡 Proposed fix to gate output and avoid the second fetch
- if ("Error at line 1: Expected [BEGIN], read [<HTML>]".equals(pe.getMessage())) { - System.out.println(); - System.out.println("It is possible that the calendar has moved."); - // Debugging: write the URL contents to standard output - try (InputStream contentStream = url.openStream()) { - System.out.printf("URL: %s%n", url); - System.out.println("Contents:"); - byte[] buffer = new byte[1024]; - int len = contentStream.read(buffer); - while (len != -1) { - System.out.write(buffer, 0, len); - len = contentStream.read(buffer); - } - System.out.println(); - } - } + if ("Error at line 1: Expected [BEGIN], read [<HTML>]".equals(pe.getMessage())) { + System.out.println(); + System.out.println("It is possible that the calendar has moved."); + if (debug) { + // Debugging: write the URL contents to standard output + try (InputStream contentStream = url.openStream()) { + System.out.printf("URL: %s%n", url); + System.out.println("Contents:"); + byte[] buffer = new byte[1024]; + int len = contentStream.read(buffer); + while (len != -1) { + System.out.write(buffer, 0, len); + len = contentStream.read(buffer); + } + System.out.println(); + } + } + }🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@src/main/java/org/plumelib/icalavailable/ICalAvailable.java` around lines 235 - 247, The new diagnostic block in ICalAvailable should only run when the existing debug flag is enabled, consistent with the other debug-only logging paths in this class. Move the URL/content dump behind the same `if (debug)` guard used elsewhere, and avoid calling `url.openStream()` a second time; instead, preserve the bytes already read from `urlStream` for parsing so the same content can be printed for debugging without a second fetch.
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Outside diff comments:
In `@src/main/java/org/plumelib/icalavailable/ICalAvailable.java`:
- Around line 235-247: The new diagnostic block in ICalAvailable should only run
when the existing debug flag is enabled, consistent with the other debug-only
logging paths in this class. Move the URL/content dump behind the same `if
(debug)` guard used elsewhere, and avoid calling `url.openStream()` a second
time; instead, preserve the bytes already read from `urlStream` for parsing so
the same content can be printed for debugging without a second fetch.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yaml
Review profile: ASSERTIVE
Plan: Pro
Run ID: 8f2b9d38-f978-450a-a0bd-9897914594e6
📒 Files selected for processing (3)
README.mdsrc/main/elisp/ical-available.elsrc/main/java/org/plumelib/icalavailable/ICalAvailable.java
💤 Files with no reviewable changes (1)
- src/main/elisp/ical-available.el
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
1 participant
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.
No description provided.