Feat/add lombok#87
Conversation
|
🚀 Hi @HossamSaberr! Thank you for contributing to MyCMD. A maintainer will review your PR shortly. 🎉 |
|
Warning Rate limit exceeded@HossamSaberr has exceeded the limit for the number of commits or files that can be reviewed per hour. Please wait 0 minutes and 30 seconds before requesting another review. ⌛ How to resolve this issue?After the wait time has elapsed, a review can be triggered using the We recommend that you space out your commits to avoid hitting the rate limit. 🚦 How do rate limits work?CodeRabbit enforces hourly rate limits for each developer per organization. Our paid plans have higher rate limits than the trial, open-source and free plans. In all cases, we re-allow further reviews after a brief timeout. Please see our FAQ for further information. 📒 Files selected for processing (1)
WalkthroughAdded Lombok to the Maven build and refactored ShellContext to use Lombok-generated accessors; introduced a final Changes
Sequence Diagram(s)(omitted — changes are structural/annotation-based and do not modify runtime control flow) Estimated code review effort🎯 3 (Moderate) | ⏱️ ~25 minutes
Poem
Pre-merge checks and finishing touches❌ Failed checks (2 warnings, 1 inconclusive)
✅ Passed checks (2 passed)
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: 1
🧹 Nitpick comments (2)
src/main/java/com/mycmd/ShellContext.java (1)
20-21: Encapsulation improvement for envVars—expose only through safe immutable views and controlled mutators.The class-level
@Getterauto-generates a publicgetEnvVars()that exposes the mutable Map directly, allowing external code to mutate internal state viagetEnvVars().put()orgetEnvVars().clear(). One existing read-only usage (SetCommand.java:13) will remain safe with an unmodifiable view; no code mutations are required.Apply the fix:
@@ - @Getter + @Getter(lombok.AccessLevel.NONE) @Setter public class ShellContext {Add safe accessors:
+ public Map<String, String> getEnvVars() { + return Collections.unmodifiableMap(envVars); + } + + public void setEnvVar(String key, String value) { + Objects.requireNonNull(key, "key"); + Objects.requireNonNull(value, "value"); + envVars.put(key, value); + } + + public void removeEnvVar(String key) { + envVars.remove(key); + }Add imports:
+import java.util.Collections; +import java.util.Objects; +import lombok.AccessLevel;pom.xml (1)
20-27: Update Lombok to latest stable 1.18.42 and wire via maven-compiler-plugin annotationProcessorPaths for consistent CI/IDE builds.Lombok 1.18.32 is compatible with Java 17 and has no known CVEs. However, the latest stable version is 1.18.42 (released Sep 18, 2025). Configuring Lombok under maven-compiler-plugin's annotationProcessorPaths ensures consistent behavior across IDE and CI builds. The suggested maven-compiler-plugin version 3.13.0 supports this configuration and works with Java 17.
Apply this (additions only) to pom.xml:
*** pom.xml @@ <properties> <maven.compiler.source>17</maven.compiler.source> <maven.compiler.target>17</maven.compiler.target> + <lombok.version>1.18.42</lombok.version> </properties> @@ <build> <plugins> + <plugin> + <groupId>org.apache.maven.plugins</groupId> + <artifactId>maven-compiler-plugin</artifactId> + <version>3.13.0</version> + <configuration> + <release>17</release> + <annotationProcessorPaths> + <path> + <groupId>org.projectlombok</groupId> + <artifactId>lombok</artifactId> + <version>${lombok.version}</version> + </path> + </annotationProcessorPaths> + </configuration> + </plugin> </plugins> </build>Optionally, drop Lombok from regular
<dependencies>and rely solely onannotationProcessorPaths, or keep it asprovidedfor IDE support—both patterns are supported.
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (2)
pom.xml(2 hunks)src/main/java/com/mycmd/ShellContext.java(2 hunks)
🔇 Additional comments (1)
src/main/java/com/mycmd/ShellContext.java (1)
3-4: Lombok imports look fine.
No issues; proceed.
|
🚀 Hi @HossamSaberr! Thank you for contributing to MyCMD. A maintainer will review your PR shortly. 🎉 |
|
🚀 Hi @HossamSaberr! Thank you for contributing to MyCMD. A maintainer will review your PR shortly. 🎉 |
There was a problem hiding this comment.
Actionable comments posted: 4
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (2)
pom.xml(2 hunks)src/main/java/com/mycmd/ShellContext.java(4 hunks)
🚧 Files skipped from review as they are similar to previous changes (1)
- pom.xml
🧰 Additional context used
🪛 GitHub Actions: ⚙️ Java CI Build
src/main/java/com/mycmd/ShellContext.java
[error] 46-46: Override annotation error: method does not override or implement a method from a supertype.
[error] 51-51: Override annotation error: method does not override or implement a method from a supertype.
[error] 56-56: Override annotation error: method does not override or implement a method from a supertype.
🔇 Additional comments (9)
src/main/java/com/mycmd/ShellContext.java (9)
3-5: LGTM! Lombok imports are correct.The Lombok imports are properly added and all are used in the class.
13-15: LGTM! Field-level annotations correctly applied.The field-level
@Setterand@NonNulloncurrentDircorrectly implement the past review suggestion, generatingsetCurrentDir(File)with null checks while avoiding unwanted setters on collection fields.
24-24: LGTM! Environment variable field properly initialized.The
envVarsfield is correctly declared asfinalwith inline initialization, preventing reassignment while allowing controlled mutations through dedicated accessor methods.
26-33: LGTM! Constructor properly initializes all fields.The constructor correctly initializes all fields and loads persisted aliases.
35-41: LGTM! History management correctly implemented.The method properly adds commands to both history lists and enforces the maximum history limit.
62-83: LGTM! Alias and history management methods are correct.All methods properly manage aliases and history with appropriate persistence calls.
85-92: LGTM! Environment variable accessors correctly implemented.The
setEnvVarandgetEnvVarmethods provide controlled access to the environment variables map with appropriate encapsulation.
95-133: LGTM! Alias persistence methods are well-implemented.Both
loadAliases()andsaveAliases()properly handle file I/O with try-with-resources, appropriate error handling, and user-friendly warning messages.
135-149: LGTM! Path resolution logic correctly implemented.The
resolvePathmethod properly handles null, empty, absolute, and relative paths with appropriate defaults.
| import java.util.*; | ||
| import java.time.Instant; | ||
|
|
||
| @Getter |
There was a problem hiding this comment.
Fix: Class-level @Getter conflicts with custom getter implementations.
The class-level @Getter generates getters for all fields, but you're attempting to provide custom implementations for getHistory(), getAliases(), and getEnvVars() that return defensive copies. This approach causes the @Override compilation errors on lines 46, 51, and 56 because you cannot override methods generated in the same class.
Solution: Exclude specific fields from the class-level @Getter by annotating them with @Getter(AccessLevel.NONE), then provide your custom getters without @Override.
Apply this diff:
+import lombok.AccessLevel;
+
@Getter
public class ShellContext {
@Setter
@NonNull
private File currentDir;
+ @Getter(AccessLevel.NONE)
private List<String> history;
+ @Getter(AccessLevel.NONE)
private Map<String, String> aliases;
private static final String ALIAS_FILE = ".mycmd_aliases";
private static final int MAX_HISTORY = 100;
private final List<String> commandHistory;
private final Instant startTime;
+ @Getter(AccessLevel.NONE)
private final Map<String, String> envVars = new HashMap<>();Then remove the @Override annotations from lines 46, 51, and 56 (see separate comments below).
🤖 Prompt for AI Agents
In src/main/java/com/mycmd/ShellContext.java around line 10, the class-level
@Getter generates getters for all fields which conflicts with your custom
getHistory(), getAliases(), and getEnvVars() methods; annotate the specific
fields (history, aliases, envVars) with @Getter(AccessLevel.NONE) to exclude
them from Lombok generation and keep the class-level @Getter for others, then
remove the @Override annotations from your custom getter methods on lines ~46,
~51, and ~56 so they become regular methods returning defensive copies.
| /** * OVERRIDES Lombok's generated getter to return a DEFENSIVE COPY | ||
| * to prevent external code from modifying the shell directly. | ||
| */ | ||
| @Override | ||
| public List<String> getHistory() { | ||
| return new ArrayList<>(history); | ||
| } |
There was a problem hiding this comment.
Critical: Remove invalid @OverRide annotation (compilation error).
Line 46's @Override annotation causes a compilation error because Lombok generates getHistory() in the same class, not in a supertype. You cannot override a method generated in the same class.
Fix: After applying the @Getter(AccessLevel.NONE) fix on the history field (see comment on line 10), remove the @Override annotation and update the misleading comment:
- /** * OVERRIDES Lombok's generated getter to return a DEFENSIVE COPY
- * to prevent external code from modifying the shell directly.
+ /**
+ * Returns a defensive copy of the command history
+ * to prevent external code from modifying it directly.
*/
- @Override
public List<String> getHistory() {
return new ArrayList<>(history);
}📝 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.
| /** * OVERRIDES Lombok's generated getter to return a DEFENSIVE COPY | |
| * to prevent external code from modifying the shell directly. | |
| */ | |
| @Override | |
| public List<String> getHistory() { | |
| return new ArrayList<>(history); | |
| } | |
| /** | |
| * Returns a defensive copy of the command history | |
| * to prevent external code from modifying it directly. | |
| */ | |
| public List<String> getHistory() { | |
| return new ArrayList<>(history); | |
| } |
🧰 Tools
🪛 GitHub Actions: ⚙️ Java CI Build
[error] 46-46: Override annotation error: method does not override or implement a method from a supertype.
🤖 Prompt for AI Agents
In src/main/java/com/mycmd/ShellContext.java around lines 43 to 49, remove the
invalid @Override annotation from the getHistory() method (it causes a
compilation error because Lombok generates the getter in the same class), and
update the Javadoc to remove the "OVERRIDES" wording — instead document that
this is a custom getter that returns a defensive copy to prevent external
mutation; keep the method body returning new ArrayList<>(history) and ensure you
have previously added @Getter(AccessLevel.NONE) to the history field so Lombok
does not generate a conflicting getter.
|
🚀 Hi @HossamSaberr! Thank you for contributing to MyCMD. A maintainer will review your PR shortly. 🎉 |
There was a problem hiding this comment.
Actionable comments posted: 2
♻️ Duplicate comments (2)
src/main/java/com/mycmd/ShellContext.java (2)
10-18: Apply the previously recommended fix: add @Getter(AccessLevel.NONE) for fields with custom getters.The past review (marked as "✅ Addressed") recommended annotating
historyandaliasesfields with@Getter(AccessLevel.NONE)to explicitly exclude them from Lombok's class-level getter generation, since you provide custom defensive-copy getters. This recommendation was not applied in the current code.While Lombok may skip generation when a method exists, explicit exclusion makes the intent clear and follows the agreed-upon fix.
Apply this diff:
+import lombok.AccessLevel; + @Getter public class ShellContext { @Setter @NonNull private File currentDir; + @Getter(AccessLevel.NONE) private List<String> history; + @Getter(AccessLevel.NONE) private Map<String, String> aliases;
45-48: Fix incorrect comment: getHistory() returns a List, not a Map.The comment on line 45 states "Returns a DEFENSIVE COPY instead of the raw Map" but
getHistory()returns aList<String>, not a Map.Apply this diff:
- /** * RETAINED FOR SAFETY: Returns a DEFENSIVE COPY instead of the raw Map. */ + /** Returns a defensive copy of the command history to prevent external modification. */ public List<String> getHistory() { return new ArrayList<>(history); }
🧹 Nitpick comments (1)
src/main/java/com/mycmd/ShellContext.java (1)
81-87: Consider adding null checks for environment variable accessors.The
setEnvVarandgetEnvVarmethods provide straightforward access to environment variables. Consider adding null checks or validation if null keys/values should not be allowed. The current implementation permits null keys and values via HashMap's default behavior.Example with validation:
public void setEnvVar(String key, String value) { + if (key == null || key.trim().isEmpty()) { + throw new IllegalArgumentException("Environment variable key cannot be null or empty"); + } envVars.put(key, value); }
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (1)
src/main/java/com/mycmd/ShellContext.java(4 hunks)
🔇 Additional comments (2)
src/main/java/com/mycmd/ShellContext.java (2)
3-5: LGTM: Lombok imports are correct.The Lombok annotation imports are appropriate for the features used in this class.
50-56: Good practice: defensive copies protect internal state.The custom getters for
getAliases()andgetEnvVars()correctly return defensive copies to prevent external code from modifying internal collections.
| private final List<String> commandHistory; | ||
| private final Instant startTime; | ||
|
|
||
| private final Map<String, String> envVars = new HashMap<>(); |
There was a problem hiding this comment.
Add @Getter(AccessLevel.NONE) to the envVars field.
The envVars field has a custom getter at lines 54-56 that returns a defensive copy. Consistent with the recommendation for history and aliases, this field should also be annotated with @Getter(AccessLevel.NONE).
Apply this diff:
+ @Getter(AccessLevel.NONE)
private final Map<String, String> envVars = new HashMap<>();📝 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.
| private final Map<String, String> envVars = new HashMap<>(); | |
| @Getter(AccessLevel.NONE) | |
| private final Map<String, String> envVars = new HashMap<>(); |
🤖 Prompt for AI Agents
In src/main/java/com/mycmd/ShellContext.java around line 24, the envVars field
needs to prevent Lombok from generating a getter because a custom defensive-copy
getter already exists; add the annotation @Getter(AccessLevel.NONE) to the
private final Map<String, String> envVars = new HashMap<>(); and ensure
lombok.Getter and lombok.AccessLevel are imported (or existing imports include
them), so the generated getter is suppressed and the class uses the custom
getter at lines 54-56.
|
🚀 Hi @HossamSaberr! Thank you for contributing to MyCMD. A maintainer will review your PR shortly. 🎉 |
|
🚀 Hi @HossamSaberr! Thank you for contributing to MyCMD. A maintainer will review your PR shortly. 🎉 |
anshumanjadiya1102
added
hacktoberfest-accepted
2 participants
close #52
Summary by CodeRabbit
Chores
Refactor