[commands] Compound Parser #216
Conversation
…122-compoundparser
…122-compoundparser
…122-compoundparser # Conflicts: # commands/src/main/java/net/apartium/cocoabeans/commands/AbstractCommandProcessingContext.java
Closes #122
…122-compoundparser # Conflicts: # CHANGELOG.md # commands/src/main/java/net/apartium/cocoabeans/commands/RegisteredCommand.java
…122-compoundparser # Conflicts: # CHANGELOG.md # commands/spigot-platform/src/test/java/net/apartium/cocoabeans/commands/spigot/parsers/Meow.java # commands/spigot-platform/src/test/java/net/apartium/cocoabeans/commands/spigot/parsers/MeowCommandTest.java # commands/spigot-platform/src/test/java/net/apartium/cocoabeans/commands/spigot/parsers/MeowParser.java # commands/src/main/java/net/apartium/cocoabeans/commands/requirements/Requirement.java # commands/src/main/java/net/apartium/cocoabeans/commands/requirements/RequirementFactory.java # commands/src/main/java/net/apartium/cocoabeans/commands/requirements/RequirementSet.java
Closes #122
Closes #122
Closes #122
WalkthroughThe pull request encompasses a series of enhancements and additions to the command handling and parsing infrastructure. Key updates include the introduction of new parser classes and interfaces, improvements to command serialization and error handling, and the addition of utility methods. Documentation has been updated to reflect these changes, and a new table of contents entry for the "Compound Parser" has been added. The modifications aim to improve the clarity and functionality of command processing and parsing mechanisms. Changes
Possibly related PRs
Suggested labels
Suggested reviewers
📜 Recent review detailsConfiguration used: CodeRabbit UI 📒 Files selected for processing (4)
🚧 Files skipped from review as they are similar to previous changes (4)
Thank you for using CodeRabbit. We offer it for free to the OSS community and would appreciate your support in helping us grow. If you find it useful, would you consider giving us a shout-out on your favorite social media? 🪧 TipsChatThere are 3 ways to chat with CodeRabbit:
Note: Be mindful of the bot's finite context window. It's strongly recommended to break down tasks such as reading entire modules into smaller chunks. For a focused discussion, use review comments to chat about specific files and their changes, instead of using the PR comments. CodeRabbit Commands (Invoked using PR comments)
Other keywords and placeholders
CodeRabbit Configuration File (
|
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 22
🧹 Outside diff range and nitpick comments (54)
commands/src/main/java/net/apartium/cocoabeans/commands/parsers/ParserVariants.java (1)
5-6: Add Javadoc documentation for better API understanding.While the annotation metadata is correctly configured, adding documentation would help users understand:
- The purpose of this container annotation
- Its relationship with
ParserVariant- Usage examples in the context of compound parsing
Example documentation:
+/** + * A container annotation that allows multiple {@link ParserVariant} annotations + * to be applied to a single method. This is primarily used in compound parsers + * to define multiple parsing variants for a single parsing method. + * + * @see ParserVariant + * @see CompoundParser + */ @Target({ElementType.METHOD}) @Retention(RetentionPolicy.RUNTIME)commands/src/main/java/net/apartium/cocoabeans/commands/parsers/ParserVariant.java (1)
10-11: Consider documenting the annotation elements.While the implementation is correct, adding Javadoc comments would help users understand:
- The purpose and expected format of the
value()string- How the
priority()affects parser selectionAdd documentation like this:
+ /** + * The pattern or format this parser variant handles. + * @return the pattern string for this parser variant + */ String value(); + /** + * The priority of this parser variant when multiple variants match. + * Higher values indicate higher priority. + * @return the priority value, defaults to 0 + */ int priority() default 0;commands/spigot-platform/src/test/java/net/apartium/cocoabeans/commands/spigot/parsers/Meow.java (3)
3-3: Consider improving field naming and adding validationThe record structure looks good, but consider these improvements:
- Rename
cattonamefor clarity- Add validation for the age field to prevent negative values
- Add
@NotNullannotations for the non-primitive fields-public record Meow(String cat, int age, Gender gender) { +public record Meow(@NotNull String name, int age, @NotNull Gender gender) { + public Meow { + if (age < 0) { + throw new IllegalArgumentException("Age cannot be negative"); + } + }
5-7: Add JavaDoc documentation for the Gender enumThe enum values are well-chosen, but adding documentation would improve code clarity.
+ /** + * Represents the gender of a cat. + */ public enum Gender { + /** Male gender */ MALE, + /** Female gender */ FEMALE, + /** Other/unspecified gender */ OTHER }
9-12: Optimize toString implementationThe current implementation uses string concatenation which could be inefficient. Consider using String.format or StringBuilder for better performance.
- public String toString() { - return "cat: " + cat + ", age: " + age + ", gender: " + gender; + public String toString() { + return String.format("cat: %s, age: %d, gender: %s", cat, age, gender); + }commands/src/main/java/net/apartium/cocoabeans/commands/GenericNode.java (3)
5-11: Enhance documentation with more details about the interface's roleWhile the documentation provides basic information, it would be beneficial to add:
- The purpose and benefits of using this interface
- Examples of typical implementations
- Design considerations for implementers
/** * A generic node interface - * Is use for objects that are related to commands or other commands related objects like compound parsers and flags + * Is used for objects that are related to commands or other command-related components such as compound parsers and flags. + * + * This interface serves as a common base type for various node implementations in the command framework, + * enabling more flexible and type-safe command processing. + * + * Typical implementations include: + * - Command nodes + * - Compound parser nodes + * - Flag nodes * * @see CommandNode * @see net.apartium.cocoabeans.commands.parsers.CompoundParser */
13-15: Consider adding common methods that would benefit all node typesThe interface is currently empty, which is fine for a marker interface. However, since this is a base interface for command-related components, consider if there are any common methods that could be beneficial across all implementations, such as:
- Node identification/naming
- Validation methods
- Common metadata
Would you like suggestions for specific methods that could be added?
7-7: Fix grammatical error in documentationThere's a grammatical error in the documentation.
- * Is use for objects that are related to commands or other commands related objects like compound parsers and flags + * Is used for objects that are related to commands or other command-related objects like compound parsers and flagscommands/spigot-platform/src/test/java/net/apartium/cocoabeans/commands/spigot/parsers/CatCommand.java (1)
8-9: Add test documentation and follow test naming conventions.Since this is a test class, consider:
- Adding Javadoc to explain the test purpose and how it demonstrates the Compound Parser functionality
- Renaming the class to
CatCommandTestto follow test naming conventions+/** + * Test class demonstrating the usage of Compound Parser with a simple cat command. + * This test validates the parsing of compound objects (Meow) through command parameters. + */ @Command("cat") -public class CatCommand implements CommandNode { +public class CatCommandTest implements CommandNode {commands/spigot-platform/src/test/java/net/apartium/cocoabeans/commands/spigot/parsers/MeowCommandTest.java (2)
10-10: Consider making the player field finalSince this field is initialized in the setup method and never changed afterwards, marking it as final would better express its immutability.
- private PlayerMock player; + private final PlayerMock player;
16-17: Improve setup method robustness and clarity
- The magic number
0passed toMeowParsershould be explained or extracted to a named constant.- Consider adding verification that command registration succeeded.
+ private static final int DEFAULT_MEOW_PARSER_PRIORITY = 0; @Override @BeforeEach public void setup() { super.setup(); - commandManager.registerArgumentTypeHandler(new MeowParser(0)); + commandManager.registerArgumentTypeHandler(new MeowParser(DEFAULT_MEOW_PARSER_PRIORITY)); commandManager.addCommand(new CatCommand()); + assertThat(commandManager.getCommands()).contains(instanceOf(CatCommand.class));commands/src/test/java/net/apartium/cocoabeans/commands/parsers/compound/PositionParser.java (3)
8-11: Add class and constant documentationConsider adding Javadoc to:
- Explain the purpose and usage of the PositionParser class
- Document the DEFAULT_KEYWORD constant and why "position" was chosen
+/** + * A compound parser that parses various position formats into Position objects. + * Supports parsing both double and integer coordinates with different variants. + */ public class PositionParser extends CompoundParser<Position> { + /** + * The default keyword used to identify position commands. + * Example usage: "position 1 2 3" + */ public static final String DEFAULT_KEYWORD = "position";
12-18: Add parameter validation and documentation to constructorsConsider the following improvements:
- Document the valid range and purpose of the priority parameter
- Add validation for the priority parameter
- Document why SimpleArgumentMapper and SimpleCommandLexer are used
+ /** + * Creates a new PositionParser with the specified priority and keyword. + * @param priority the parser priority (higher values take precedence) + * @param keyword the keyword to identify this parser + * @throws IllegalArgumentException if priority is negative + */ public PositionParser(int priority, String keyword) { + if (priority < 0) throw new IllegalArgumentException("Priority must be non-negative"); + if (keyword == null) throw new IllegalArgumentException("Keyword cannot be null"); super(keyword, Position.class, priority, new SimpleArgumentMapper(), new SimpleCommandLexer()); }
8-33: Consider enhancing coordinate handling flexibilityArchitectural suggestions:
- Consider making the coordinate scaling configurable through constructor parameters
- Add coordinate range validation if there are valid bounds for the Position
- Consider adding support for relative coordinates (e.g., ~1 ~2 ~3 syntax common in game commands)
commands/spigot-platform/src/test/java/net/apartium/cocoabeans/commands/spigot/parsers/MeowParser.java (1)
16-18: Consider dependency injection for mapper and lexerInstead of creating new instances of
SimpleArgumentMapperandSimpleCommandLexerin the constructor, consider injecting them as constructor parameters. This would:
- Improve testability
- Allow for different implementations
- Follow dependency injection principles
- public MeowParser(int priority) { - super("meow", Meow.class, priority, new SimpleArgumentMapper(), new SimpleCommandLexer()); + public MeowParser(int priority, ArgumentMapper mapper, CommandLexer lexer) { + super("meow", Meow.class, priority, mapper, lexer);commands/src/main/java/net/apartium/cocoabeans/commands/ArgumentMapper.java (1)
30-30: Consider enhancing method documentationWhile the existing documentation is clear, consider adding a note about the
RegisteredVariant.Parametertype to help users understand its relationship with the command registration system.Add this to the
@param parametersdocumentation:- * @param parameters parameters + * @param parameters An array of RegisteredVariant.Parameter objects that define the command's parameter specificationscommands/src/main/java/net/apartium/cocoabeans/commands/CommandNode.java (1)
Line range hint
18-35: Enhance interface documentationWhile the implementation is correct, the class-level documentation could be more descriptive about:
- The relationship with GenericNode
- The purpose of command nodes in the command system
- When to implement this interface vs using existing implementations
/** * To be implemented by command classes to provide general functionality + * + * This interface extends GenericNode to provide command-specific functionality + * while maintaining compatibility with the generic node hierarchy. It serves as + * the base for all command implementations in the system. + * + * @see GenericNode */commands/src/test/java/net/apartium/cocoabeans/commands/parsers/compound/PositionParserTest.java (2)
14-30: Enhance test coverage and documentation.While the test cases cover basic scenarios, consider the following improvements:
- Add descriptive comments for each test case to explain the scenario being tested
- Include additional edge cases:
- Maximum/minimum coordinate values
- Extremely large numbers
- Scientific notation
- Different decimal formats
32-38: Expand tab completion test coverage.Consider adding test cases for:
- Decimal point completions (e.g., "1." should suggest "1.0", "1.5", etc.)
- Empty input completion
- Invalid input handling
- Boundary cases for coordinate suggestions
commands/src/main/java/net/apartium/cocoabeans/commands/requirements/argument/RangeArgumentRequirementFactory.java (2)
15-15: Remove unused CommandNode importSince the class now uses
GenericNode, theCommandNodeimport appears to be unused and can be removed.-import net.apartium.cocoabeans.commands.CommandNode;
Line range hint
34-34: Consider adding constructor validation for RangeImplThe
RangeImplrecord looks good, but consider adding validation in the compact constructor to ensurestep > 0andfrom < to. This would prevent invalid range configurations.- public record RangeImpl(double from, double to, double step) implements ArgumentRequirement { + public record RangeImpl(double from, double to, double step) implements ArgumentRequirement { + public RangeImpl { + if (step <= 0) throw new IllegalArgumentException("Step must be positive"); + if (from >= to) throw new IllegalArgumentException("From must be less than to"); + }CHANGELOG.md (1)
11-11: Consider expanding the Compound Parser changelog entryThe current entry "Add Compound parser" could be more descriptive. Consider expanding it to better reflect the feature's purpose and benefits, as described in the PR objectives.
-[commands] Add Compound parser +[commands] Add Compound Parser - enables creation of custom parsers composed of other parsers for flexible command input handlingcommands/src/main/java/net/apartium/cocoabeans/commands/parsers/CompoundParserBranchProcessor.java (3)
11-13: Consider a more descriptive field nameThe field name
objectMapis quite generic. Consider renaming it to something more descriptive likeparserOptionsWithRequirementsorrequirementToParserMappingto better reflect its purpose.
40-62: Improve validation, documentation, and code claritySeveral improvements could be made to the tab completion logic:
- Add input validation
- Document the purpose of highestIndex
- Consider using a constant instead of magic number -1
- Consider using Stream API for more concise collection processing
Here's a suggested improvement:
+ private static final int NO_COMPLETION_INDEX = -1; + + /** + * Generates tab completion suggestions based on the command context. + * @param processingContext The context containing command information + * @return Optional containing tab completion result if suggestions are available + * @throws NullPointerException if processingContext is null + */ public Optional<ArgumentParser.TabCompletionResult> tabCompletion(CommandProcessingContext processingContext) { + Objects.requireNonNull(processingContext, "processingContext cannot be null"); RequirementEvaluationContext requirementEvaluationContext = new RequirementEvaluationContext( processingContext.sender(), processingContext.label(), processingContext.args().toArray(new String[0]), processingContext.index()); Set<String> result = new HashSet<>(); - int highestIndex = -1; + int highestIndex = NO_COMPLETION_INDEX;
11-65: Consider architectural improvementsThe class currently handles both parsing and tab completion responsibilities. Consider:
- Splitting these into separate classes following Single Responsibility Principle
- Implementing a builder pattern for configuring parser options and requirements
- Adding an interface to define the contract for parser processors
This would improve maintainability and make the code more testable.
commands/src/main/java/net/apartium/cocoabeans/commands/parsers/SourceParserFactory.java (1)
57-57: Consider documenting caching behavior with GenericNode.The migration to
GenericNodeis correct. However, since this class implements caching behavior, it would be valuable to:
- Document any performance implications of using
GenericNode- Verify that caching behavior remains optimal with the new type
Consider adding Javadoc to explain:
/** * Implementation of MapBasedParser that supports result caching. * @param <T> The type of values in the map * @param node The generic node used for result generation * @param resultMaxAgeInMills Controls cache invalidation: * -1 for infinite cache, * 0 for no cache, * >0 for time-based cache in milliseconds */Also applies to: 65-65
commands/spigot-platform/src/main/java/net/apartium/cocoabeans/commands/spigot/requirements/factory/PermissionFactory.java (2)
34-34: Address TODO comment regarding inverted functionalityThe TODO comment suggests that the inverted functionality might be incomplete or needs attention.
Would you like me to help implement the inverted functionality or create a GitHub issue to track this task?
Line range hint
41-54: Consider consolidating duplicate error messagesThe same error message "You don't have permission to execute this command" is duplicated in two places. Consider extracting it to a constant or using a helper method to improve maintainability.
+ private static final String NO_PERMISSION_MESSAGE = "You don't have permission to execute this command"; @Override public RequirementResult meetsRequirement(RequirementEvaluationContext context) { Sender sender = context.sender(); if ((sender.getSender() == null || !(sender.getSender() instanceof CommandSender commandSender))) { return RequirementResult.error(new UnmetPermissionResponse( this, context, - "You don't have permission to execute this command" + NO_PERMISSION_MESSAGE )); } if (!commandSender.hasPermission(permissionAsString)) return RequirementResult.error(new UnmetPermissionResponse( this, context, - "You don't have permission to execute this command" + NO_PERMISSION_MESSAGE ));commands/spigot-platform/src/test/java/net/apartium/cocoabeans/commands/spigot/game/commands/utils/InGameFactory.java (1)
Line range hint
45-73: Consider standardizing error messages for consistency.The error handling is comprehensive, but there are two identical error messages for different scenarios:
- When gamePlayer is null
- When game is null
Both return: "Player isn't in any game"
Consider differentiating these messages for better debugging:
- "Player isn't in any game" + "Player exists but is not registered in any game"- "Player isn't in any game" + "Player is registered but not associated with any active game"commands/src/main/java/net/apartium/cocoabeans/commands/parsers/WithParserFactory.java (1)
100-106: Consider adding debug logging for parser instantiationFor better observability during development and troubleshooting, consider adding debug-level logging before instantiating the parser.
try { + SharedSecrets.LOGGER.log(System.Logger.Level.DEBUG, + "Instantiating parser {} with priority {} and keyword {}", + withParser.value().getName(), withParser.priority(), withParser.keyword()); ArgumentParser<?> argumentParser = newInstance( ConstructorUtils.getDeclaredConstructors(withParser.value()), withParser.priority(), withParser.keyword() );commands/spigot-platform/src/main/java/net/apartium/cocoabeans/commands/spigot/requirements/factory/WhitelistRequirementFactory.java (2)
14-14: Consider removing unused CommandNode importSince the method signature now uses GenericNode, the CommandNode import might no longer be needed.
-import net.apartium.cocoabeans.commands.CommandNode;
Line range hint
71-75: Improve error message professionalismThe error message "Invert console by pass lolllllllllll" appears unprofessional and should be replaced with a clear, descriptive message.
- invert ? "Invert console by pass lolllllllllll" : "Sender is not a player" + invert ? "Console access is not allowed in blacklist mode" : "Sender is not a player"commands/src/main/java/net/apartium/cocoabeans/commands/requirements/ArgumentRequirementFactory.java (2)
36-62: Solid implementation with room for minor improvementsThe implementation is well-structured with proper null handling and error checking. Consider these optimizations:
- Use pre-sized array in toArray for better performance
- Simplify the nested Optional usage
- return result.toArray(new ArgumentRequirement[0]); + return result.toArray(new ArgumentRequirement[result.size()]); - ArgumentRequirement argumentRequirement = Optional.ofNullable(argumentRequirementFactories.computeIfAbsent( - argumentRequirementType, - clazz -> ArgumentRequirementFactory.createFromAnnotation(annotation) - )) - .map(factory -> factory.getArgumentRequirement(node, annotation)) - .orElse(null); + ArgumentRequirementFactory factory = argumentRequirementFactories.computeIfAbsent( + argumentRequirementType, + clazz -> ArgumentRequirementFactory.createFromAnnotation(annotation) + ); + if (factory == null) continue; + + ArgumentRequirement argumentRequirement = factory.getArgumentRequirement(node, annotation);
Line range hint
85-102: Consider more specific exception handlingWhile the implementation is cleaner after removing the CommandManager dependency, consider handling specific exceptions separately instead of wrapping them all in RuntimeException.
- } catch (InstantiationException | IllegalAccessException | InvocationTargetException | NoSuchMethodException e) { - throw new RuntimeException("Failed to instantiate argument requirement factory: " + clazz, e); + } catch (NoSuchMethodException e) { + throw new IllegalStateException("No default constructor found in " + clazz.getName(), e); + } catch (InstantiationException | IllegalAccessException e) { + throw new IllegalStateException("Failed to access or instantiate " + clazz.getName(), e); + } catch (InvocationTargetException e) { + throw new RuntimeException("Constructor threw an exception in " + clazz.getName(), e.getCause());Writerside/topics/compound-parser.md (3)
94-140: Fix markdown formatting issues for better documentation structure.Several markdown formatting issues need attention:
- Heading levels should increment by one level at a time (h4 at line 94 should be h3)
- List items should have consistent indentation (lines 138-140)
- Unordered lists should follow proper indentation rules (lines 91-92, 137)
Apply these changes to fix the formatting:
-#### How could we warp it? +### How could we wrap it? -That will save you memory and computation time but the parsers you use must support multithreading +That will save you memory and computation time, but the parsers you use must support multithreading - **Improved Readability** 📖: The compound parser provides... -**Easier to Use** 👍: The compound parser is easy... -**Reduced Code** 📦: The compound parser is... -**Increased Flexibility** 🎯: The compound parser allows... + - **Improved Readability** 📖: The compound parser provides... + - **Easier to Use** 👍: The compound parser is easy... + - **Reduced Code** 📦: The compound parser is... + - **Increased Flexibility** 🎯: The compound parser allows...🧰 Tools
🪛 LanguageTool
[uncategorized] ~96-~96: You might be missing the article “a” here.
Context: ...package-private */then we will create classLocationParser` that will be public ``...(AI_EN_LECTOR_MISSING_DETERMINER_A)
[uncategorized] ~133-~133: A comma might be missing here.
Context: ...at will save you memory and computation time but the parsers you use must support mu...(AI_EN_LECTOR_MISSING_PUNCTUATION_COMMA)
[uncategorized] ~134-~134: Use a comma before ‘but’ if it connects two independent clauses (unless they are closely connected and short).
Context: ...er every time. You could lock the parser but it's not recommended because it will sl...(COMMA_COMPOUND_SENTENCE)
🪛 Markdownlint (0.35.0)
94-94: Expected: h3; Actual: h4
Heading levels should only increment by one level at a time(MD001, heading-increment)
138-138: Expected: 1; Actual: 0
Inconsistent indentation for list items at the same level(MD005, list-indent)
139-139: Expected: 1; Actual: 0
Inconsistent indentation for list items at the same level(MD005, list-indent)
140-140: Expected: 1; Actual: 0
Inconsistent indentation for list items at the same level(MD005, list-indent)
137-137: Expected: 0; Actual: 1
Unordered list indentation(MD007, ul-indent)
59-68: Consider thread-safety implications when accessing Player/World objects.The methods using
sender.getWorld()could potentially face thread-safety issues if the player changes worlds during command processing.Consider these approaches:
- Document the thread-safety assumptions
- Cache the world reference at the start of parsing
- Add synchronization if necessary
- Validate that the player is still online and in the same world before creating the Location
Example implementation:
@SenderLimit(SenderType.PLAYER) @ParserVariant("<double> <double> <double>") public Location parseWithXyz(Player sender, double x, double y, double z) { World world = sender.getWorld(); // Cache world reference if (!sender.isOnline() || !world.equals(sender.getWorld())) { throw new IllegalStateException("Player world changed during command processing"); } validateCoordinates(x, y, z); return new Location(world, x, y, z); }Also applies to: 70-81
136-140: Consider enhancing the advantages section with concrete examples.While the advantages are well-described, adding specific examples would make them more tangible for users:
Consider expanding each point like this:
## Advantages - - **Improved Readability** 📖: The compound parser provides a clear and descriptive name for each variant... + - **Improved Readability** 📖: The compound parser provides a clear and descriptive name for each variant. For example: + ```java + @ParserVariant("<world> <double> <double> <double>") // Clearly shows required arguments + ```🧰 Tools
🪛 Markdownlint (0.35.0)
138-138: Expected: 1; Actual: 0
Inconsistent indentation for list items at the same level(MD005, list-indent)
139-139: Expected: 1; Actual: 0
Inconsistent indentation for list items at the same level(MD005, list-indent)
140-140: Expected: 1; Actual: 0
Inconsistent indentation for list items at the same level(MD005, list-indent)
137-137: Expected: 0; Actual: 1
Unordered list indentation(MD007, ul-indent)
commands/src/main/java/net/apartium/cocoabeans/commands/parsers/factory/IntRangeParserFactory.java (1)
Line range hint
123-127: Review tab completion range calculationThe calculation
Math.max(targetNum * 10, from)in the tab completion logic seems arbitrary and could cause unexpected behavior:
- Multiplying targetNum by 10 could create large jumps in suggestions
- May skip valid options within the specified range
- Could be inefficient for large ranges
Consider simplifying to use the direct range values:
- for (int i = Math.max(targetNum * 10, from); i < to; i += step) { + for (int i = Math.max(targetNum, from); i < to; i += step) { allPossiblesOptions.add(i + ""); }commands/src/main/java/net/apartium/cocoabeans/commands/CommandManager.java (3)
131-131: LGTM! Consider enhancing error logging.The refactoring from
RegisteredCommandVarianttoRegisteredVariantis clean and maintains the existing functionality. The error handling is robust with proper fallback mechanisms.Consider adding debug-level logging before invoking each variant to aid in troubleshooting:
for (RegisteredVariant method : context.option().getRegisteredCommandVariants()) { + logger.debug("Attempting to invoke command variant: {}", method); try { if (invoke(context, sender, method))
189-194: Optimize stream operations for better performance.The current implementation creates an intermediate list through
stream().toList()before adding it to a new ArrayList. This can be optimized to reduce memory allocations.Consider collecting directly to the ArrayList:
- List<Object> parameters = new ArrayList<>(registeredVariant.argumentIndexList().stream() - .<Object>map((argumentIndex -> argumentIndex.get(context.toArgumentContext()))) - .toList()); + List<Object> parameters = registeredVariant.argumentIndexList().stream() + .<Object>map(argumentIndex -> argumentIndex.get(context.toArgumentContext())) + .collect(ArrayList::new, ArrayList::add, ArrayList::addAll);
Line range hint
206-210: Improve error handling in method invocation.The current implementation suppresses the original exception by only dispensing it. This could make debugging more difficult.
Consider preserving the original exception:
try { output = registeredVariant.method().invokeWithArguments(parameters); } catch (Throwable e) { - Dispensers.dispense(e); - return false; // never going to reach this place + Dispensers.dispense(e); + throw new CommandExecutionException("Failed to invoke command variant", e); }This change would:
- Maintain the error logging through the Dispenser
- Wrap the original exception with context
- Allow proper error propagation
commands/src/main/java/net/apartium/cocoabeans/commands/SimpleArgumentMapper.java (1)
Line range hint
95-146: Consider documenting the type change rationaleThe refactoring from
RegisteredCommandVarianttoRegisteredVariantsimplifies the type system. Consider adding a comment explaining this architectural decision for future maintainers.commands/src/testFixtures/java/net/apartium/cocoabeans/commands/parsers/ParserAssertions.java (2)
12-14: Consider using explicit imports instead of wildcard importsReplace the wildcard import with explicit imports for the specific classes needed from
java.util.*. This improves code readability and helps prevent potential naming conflicts.-import java.util.*; +import java.util.List; +import java.util.Objects; +import java.util.Optional; +import java.util.OptionalInt; +import java.util.Set;
318-326: Add detailed JavaDoc descriptionsThe JavaDoc is missing descriptions for:
- The method's purpose and behavior
- Each parameter's purpose and expected values
- The relationship between
expectedandexpectedIndex/** + * Asserts that the parser's tab completion results match the expected values. + * This method verifies both the completion suggestions and the new cursor position. + * * @param parser the parser to test - * @param sender - * @param label - * @param args - * @param startIndex - * @param expected + * @param sender the command sender + * @param label the command label + * @param args the command arguments + * @param startIndex the starting index for tab completion + * @param expected the expected tab completion suggestions + * @param expectedIndex the expected cursor position after tab completion */commands/src/main/java/net/apartium/cocoabeans/commands/parsers/CompoundParserOption.java (2)
42-44: Consider appending to the list instead of inserting at the beginningIn line 44, the code inserts the parse result at the beginning of the list:
.add(0, parse.get().result());Inserting at index
0shifts all existing elements, which can be inefficient for large lists. If the order of elements inmappedByClassis not critical, consider appending the result to the end of the list:.add(parse.get().result());This change can improve performance by avoiding unnecessary element shifts.
56-75: EnsurehighestIndexis appropriately initialized and updatedIn the
tabCompletionmethod,highestIndexis initialized to-1(line 59). When updatinghighestIndex(line 67), ensure that it reflects a valid index if tab completion results are found. IfhighestIndexremains-1, it may indicate that no parsers provided a higher index, which could affect the correctness of tab completions.Consider initializing
highestIndextoprocessingContext.index()to represent the current index as a baseline:int highestIndex = processingContext.index();This ensures that
highestIndexalways has a valid value corresponding to the parsing context.commands/src/main/java/net/apartium/cocoabeans/commands/requirements/RequirementFactory.java (2)
34-35: Consider returning an emptyRequirementSetinstead of nullReturning
nullwhenannotationsisnullmay lead toNullPointerExceptions in the calling code. It would be safer to return an emptyRequirementSetto simplify handling and avoid potential errors.
50-65: Evaluate the necessity ofcreateFromAnnotationmethodWith the modification above, the
createFromAnnotationmethod may become redundant or require adjustments. Consider refactoring or removing this method to streamline the code and reduce potential confusion.commands/src/main/java/net/apartium/cocoabeans/commands/RegisteredVariant.java (1)
44-45: Correct the grammatical error in the Javadoc commentThe Javadoc comment has a grammatical error that should be corrected for clarity and professionalism.
Current comment:
/** * Parameter represent a parameter of the command after has been parsed */Suggested correction:
/** * Parameter represents a parameter of the command after it has been parsed */commands/src/main/java/net/apartium/cocoabeans/commands/parsers/ParserFactory.java (1)
46-48: Consider logging the ignoredNoSuchMethodExceptionIgnoring exceptions without logging can hinder debugging efforts. Consider logging the
NoSuchMethodExceptionto aid in diagnosing potential issues when a method is not found.Apply this diff to log the exception:
} catch (NoSuchMethodException ignored) { - // ignored + // Log at fine level for debugging purposes + Logger.getLogger(ParserFactory.class.getName()).log(Level.FINE, "Method not found: " + method.getName(), ignored); }commands/src/main/java/net/apartium/cocoabeans/commands/parsers/CompoundParser.java (2)
228-229: Remove misleading comment about unreachable codeThe comment
// never going to reach this placeis misleading because the return statement will be reached if an exception is thrown. Please remove or correct the comment.Apply this diff:
- return Optional.empty(); // never going to reach this place + return Optional.empty();
137-168: Refactor 'createArgumentOption' method for improved readabilityThe
createArgumentOptionmethod is lengthy and handles multiple responsibilities, which can make it difficult to read and maintain. Consider refactoring it into smaller, focused methods to enhance readability and maintainability.commands/src/main/java/net/apartium/cocoabeans/commands/RegisteredCommand.java (2)
198-203: Consider refactoring to reduce duplication when merging method argument parsersThe process of merging argument parsers from methods and their super methods appears in multiple places. Refactoring this logic into a separate method would enhance maintainability.
Suggested refactor:
Extract a method to handle the merging of method and superclass argument parsers:
private Map<String, ArgumentParser<?>> mergeMethodArgumentParsers(ParserSubCommandContext context, MethodHandles.Lookup publicLookup) throws IllegalAccessException { Map<String, ArgumentParser<?>> methodArgumentTypeHandlerMap = new HashMap<>(ParserFactory.getArgumentParsers( context.commandNode, context.method.getAnnotations(), context.method, false, commandManager.parserFactories )); for (Method targetMethod : MethodUtils.getMethodsFromSuperClassAndInterface(context.method)) { CollectionHelpers.mergeInto( methodArgumentTypeHandlerMap, ParserFactory.getArgumentParsers( context.commandNode, targetMethod.getAnnotations(), targetMethod, false, commandManager.parserFactories ) ); } return methodArgumentTypeHandlerMap; }Then replace the existing code with a call to this new method.
386-386: Refactor duplicated logic infindAllRequirementsmethodsThe methods
findAllRequirementsforClass<?>andMethodshare similar logic for accumulating requirements. Refactoring this into a single method can adhere to the DRY principle.Suggested refactor:
Create a generic method that accepts an
AnnotatedElement:private Set<Requirement> findAllRequirements(CommandNode commandNode, AnnotatedElement element) { Set<Requirement> requirements = new HashSet<>(RequirementFactory.createRequirementSet( commandNode, element.getAnnotations(), commandManager.requirementFactories )); if (element instanceof Class<?> clazz) { for (Class<?> c : ClassUtils.getSuperClassAndInterfaces(clazz)) { requirements.addAll(RequirementFactory.createRequirementSet( commandNode, c.getAnnotations(), commandManager.requirementFactories )); } } else if (element instanceof Method method) { for (Method target : MethodUtils.getMethodsFromSuperClassAndInterface(method)) { requirements.addAll(RequirementFactory.createRequirementSet( commandNode, target.getAnnotations(), commandManager.requirementFactories )); } } return requirements; }Then replace calls to the original methods with this new method.
Also applies to: 393-395
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
📒 Files selected for processing (40)
CHANGELOG.md(2 hunks)Writerside/cb.tree(1 hunks)Writerside/topics/Examples.md(1 hunks)Writerside/topics/compound-parser.md(1 hunks)commands/spigot-platform/src/main/java/net/apartium/cocoabeans/commands/spigot/requirements/factory/PermissionFactory.java(1 hunks)commands/spigot-platform/src/main/java/net/apartium/cocoabeans/commands/spigot/requirements/factory/SenderLimitFactory.java(2 hunks)commands/spigot-platform/src/main/java/net/apartium/cocoabeans/commands/spigot/requirements/factory/WhitelistRequirementFactory.java(2 hunks)commands/spigot-platform/src/test/java/net/apartium/cocoabeans/commands/spigot/game/commands/utils/InGameFactory.java(2 hunks)commands/spigot-platform/src/test/java/net/apartium/cocoabeans/commands/spigot/parsers/CatCommand.java(1 hunks)commands/spigot-platform/src/test/java/net/apartium/cocoabeans/commands/spigot/parsers/Meow.java(1 hunks)commands/spigot-platform/src/test/java/net/apartium/cocoabeans/commands/spigot/parsers/MeowCommandTest.java(1 hunks)commands/spigot-platform/src/test/java/net/apartium/cocoabeans/commands/spigot/parsers/MeowParser.java(1 hunks)commands/src/main/java/net/apartium/cocoabeans/commands/AbstractCommandProcessingContext.java(2 hunks)commands/src/main/java/net/apartium/cocoabeans/commands/ArgumentMapper.java(1 hunks)commands/src/main/java/net/apartium/cocoabeans/commands/CommandBranchProcessor.java(1 hunks)commands/src/main/java/net/apartium/cocoabeans/commands/CommandManager.java(2 hunks)commands/src/main/java/net/apartium/cocoabeans/commands/CommandNode.java(1 hunks)commands/src/main/java/net/apartium/cocoabeans/commands/CommandOption.java(4 hunks)commands/src/main/java/net/apartium/cocoabeans/commands/GenericNode.java(1 hunks)commands/src/main/java/net/apartium/cocoabeans/commands/RegisteredCommand.java(6 hunks)commands/src/main/java/net/apartium/cocoabeans/commands/RegisteredVariant.java(1 hunks)commands/src/main/java/net/apartium/cocoabeans/commands/SimpleArgumentMapper.java(3 hunks)commands/src/main/java/net/apartium/cocoabeans/commands/parsers/CompoundParser.java(1 hunks)commands/src/main/java/net/apartium/cocoabeans/commands/parsers/CompoundParserBranchProcessor.java(1 hunks)commands/src/main/java/net/apartium/cocoabeans/commands/parsers/CompoundParserOption.java(1 hunks)commands/src/main/java/net/apartium/cocoabeans/commands/parsers/ParserFactory.java(3 hunks)commands/src/main/java/net/apartium/cocoabeans/commands/parsers/ParserVariant.java(1 hunks)commands/src/main/java/net/apartium/cocoabeans/commands/parsers/ParserVariants.java(1 hunks)commands/src/main/java/net/apartium/cocoabeans/commands/parsers/SourceParserFactory.java(4 hunks)commands/src/main/java/net/apartium/cocoabeans/commands/parsers/WithParserFactory.java(2 hunks)commands/src/main/java/net/apartium/cocoabeans/commands/parsers/factory/IntRangeParserFactory.java(2 hunks)commands/src/main/java/net/apartium/cocoabeans/commands/requirements/ArgumentRequirementFactory.java(3 hunks)commands/src/main/java/net/apartium/cocoabeans/commands/requirements/RequirementFactory.java(2 hunks)commands/src/main/java/net/apartium/cocoabeans/commands/requirements/argument/RangeArgumentRequirementFactory.java(2 hunks)commands/src/test/java/net/apartium/cocoabeans/commands/GeneralCommandTest.java(1 hunks)commands/src/test/java/net/apartium/cocoabeans/commands/WallRequirementFactory.java(1 hunks)commands/src/test/java/net/apartium/cocoabeans/commands/multilayered/PermissionFactory.java(2 hunks)commands/src/test/java/net/apartium/cocoabeans/commands/parsers/compound/PositionParser.java(1 hunks)commands/src/test/java/net/apartium/cocoabeans/commands/parsers/compound/PositionParserTest.java(1 hunks)commands/src/testFixtures/java/net/apartium/cocoabeans/commands/parsers/ParserAssertions.java(2 hunks)
🧰 Additional context used
📓 Learnings (1)
commands/src/main/java/net/apartium/cocoabeans/commands/requirements/ArgumentRequirementFactory.java (1)
Learnt from: ikfir
PR: PoweredByApartium/cocoa-beans#214
File: commands/src/main/java/net/apartium/cocoabeans/commands/requirements/ArgumentRequirementFactory.java:30-43
Timestamp: 2024-11-22T20:02:07.282Z
Learning: The `CommandManager` class already implements caching mechanisms, so additional caching within factory methods like `ArgumentRequirementFactory.create()` is unnecessary.
🪛 Markdownlint (0.35.0)
Writerside/topics/Examples.md
18-18: null
No empty links
(MD042, no-empty-links)
Writerside/topics/compound-parser.md
94-94: Expected: h3; Actual: h4
Heading levels should only increment by one level at a time
(MD001, heading-increment)
138-138: Expected: 1; Actual: 0
Inconsistent indentation for list items at the same level
(MD005, list-indent)
139-139: Expected: 1; Actual: 0
Inconsistent indentation for list items at the same level
(MD005, list-indent)
140-140: Expected: 1; Actual: 0
Inconsistent indentation for list items at the same level
(MD005, list-indent)
91-91: Expected: 0; Actual: 1
Unordered list indentation
(MD007, ul-indent)
92-92: Expected: 0; Actual: 1
Unordered list indentation
(MD007, ul-indent)
137-137: Expected: 0; Actual: 1
Unordered list indentation
(MD007, ul-indent)
🪛 LanguageTool
Writerside/topics/compound-parser.md
[uncategorized] ~96-~96: You might be missing the article “a” here.
Context: ...package-private */then we will create classLocationParser` that will be public ``...
(AI_EN_LECTOR_MISSING_DETERMINER_A)
[uncategorized] ~133-~133: A comma might be missing here.
Context: ...at will save you memory and computation time but the parsers you use must support mu...
(AI_EN_LECTOR_MISSING_PUNCTUATION_COMMA)
[uncategorized] ~134-~134: Use a comma before ‘but’ if it connects two independent clauses (unless they are closely connected and short).
Context: ...er every time. You could lock the parser but it's not recommended because it will sl...
(COMMA_COMPOUND_SENTENCE)
🔇 Additional comments (52)
commands/src/main/java/net/apartium/cocoabeans/commands/parsers/ParserVariants.java (2)
1-3: LGTM! Package and imports are well-structured.
The package naming follows Java conventions, and the import statement appropriately includes all required annotation types.
7-11: Verify the repeatable annotation setup.
The container annotation is correctly structured. Let's verify that ParserVariant is properly configured as a repeatable annotation that works with this container.
✅ Verification successful
Repeatable annotation setup is correctly configured
The ParserVariant annotation is properly set up with:
@Repeatable(ParserVariants.class)marking it as repeatable with the correct container- Required
value()method returning String - Additional
priority()method with a default value
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash
# Description: Verify that ParserVariant is properly configured as a repeatable annotation
# Check if ParserVariant is marked as @Repeatable with ParserVariants as the container
ast-grep --pattern $'@Repeatable($_)
@interface ParserVariant'
# Check for the presence of required value() method in ParserVariant
ast-grep --pattern $'@interface ParserVariant {
$$$
String value();
$$$
}'
Length of output: 145
Script:
#!/bin/bash
# Let's try to locate and examine the ParserVariant annotation file first
fd "ParserVariant.java" --type f --exec cat {}
Length of output: 319
commands/src/main/java/net/apartium/cocoabeans/commands/parsers/ParserVariant.java (2)
5-8: LGTM! Well-designed annotation metadata.
The annotation is properly configured with appropriate metadata:
- Method-level target ensures correct usage in parser methods
- Runtime retention enables reflection-based parser implementation
- Repeatable design allows multiple variants per method
1-13: Verify consistent usage across parser implementations.
Let's ensure the annotation is being used consistently across different parsers.
✅ Verification successful
ParserVariant annotation is used consistently and correctly
The verification shows consistent usage patterns:
- All
@ParserVariantannotations properly specify input parameter types in angle brackets - Used in test implementations (MeowParser, PositionParser) and documentation examples
- No inconsistent priority values found in
@ParserVariantusages - Annotation is correctly applied at method level with appropriate parameter mappings
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash
# Description: Check usage patterns of ParserVariant annotation
# Find all parser implementations
echo "=== Parser Implementations ==="
ast-grep --pattern 'class $_ implements Parser<$_> { $$$ }'
# Find methods annotated with ParserVariant
echo -e "\n=== ParserVariant Usage ==="
rg -B 1 -A 3 "@ParserVariant"
# Check priority patterns
echo -e "\n=== Priority Patterns ==="
rg "priority\s*=\s*\d+"
Length of output: 5431
commands/spigot-platform/src/test/java/net/apartium/cocoabeans/commands/spigot/parsers/Meow.java (1)
1-13: Verify test coverage for this model class
Since this class is in the test directory, let's verify its usage in test cases.
✅ Verification successful
Let me gather more information about the MeowParser and MeowCommandTest to verify the test coverage.
Let me check the CatCommand implementation to complete the verification of test coverage.
Test coverage for Meow model class is adequate
The verification shows that the Meow class is properly integrated into the test suite with:
- A dedicated parser (MeowParser) that handles the compound parsing with the format
<string> <int> <gender> - A test command (CatCommand) that uses the Meow class in its
setsubcommand - A test class (MeowCommandTest) that verifies the parsing and command execution
- Proper test setup with registration of both the parser and command
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash
# Search for test cases using the Meow class
rg -l "class \w+Test.*\{" | xargs rg "Meow"
# Search for parser implementations using this class
ast-grep --pattern 'class $_ implements Parser<Meow> { $$$ }'
Length of output: 2082
Script:
#!/bin/bash
# Check the MeowParser implementation
rg -A 10 "class MeowParser"
# Check the test implementation in MeowCommandTest
rg -A 20 "class MeowCommandTest"
# Look for any test methods in MeowCommandTest
ast-grep --pattern 'class MeowCommandTest {
$$$
@Test
$_ $_($$$) {
$$$
}
$$$
}'
Length of output: 4425
Script:
#!/bin/bash
# Check the CatCommand implementation
rg -A 15 "class CatCommand"
# Look for additional test methods that might be in separate files
rg -l "class.*Test" commands/spigot-platform/src/test/ | xargs rg "cat.*set.*meow"
Length of output: 1164
commands/spigot-platform/src/test/java/net/apartium/cocoabeans/commands/spigot/parsers/CatCommand.java (1)
1-7: LGTM!
Package declaration and imports are clean and appropriate.
commands/spigot-platform/src/test/java/net/apartium/cocoabeans/commands/spigot/parsers/MeowCommandTest.java (2)
1-8: LGTM!
The class structure and imports are well-organized and follow testing best practices.
22-26: Verify complete test coverage of CatCommand
Let's verify if we have all the necessary test cases for the CatCommand implementation.
✅ Verification successful
Let me try a different search strategy to find the command implementations and test coverage.
Let's check the content of the CatCommand class to understand all available commands that need testing.
Test coverage is complete for CatCommand
The CatCommand class only has one subcommand set <string> <meow> which is already covered by the existing test case setCommand(). The test verifies the command with appropriate parameters: an ID string ("tom") and a Meow object constructed from "a_cat 13 male".
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash
# Description: Find all available cat command implementations to ensure complete test coverage
# Search for CatCommand class implementation
echo "Searching for CatCommand implementation..."
ast-grep --pattern 'class CatCommand {
$$$
}'
# Search for command methods in CatCommand
echo "Searching for command methods..."
ast-grep --pattern '@Command($$$)
$$$'
Length of output: 260
Script:
#!/bin/bash
# Let's search for files related to cat commands and their tests
echo "Searching for cat-related command files..."
fd -e java cat
# Let's also look for command annotations and implementations
echo -e "\nSearching for command implementations..."
rg -l "class.*Cat.*Command" -g "*.java"
# Get content of the test file to analyze test coverage
echo -e "\nAnalyzing test file content..."
cat commands/spigot-platform/src/test/java/net/apartium/cocoabeans/commands/spigot/parsers/MeowCommandTest.java
Length of output: 2044
Script:
#!/bin/bash
# Get the content of CatCommand implementation
echo "Analyzing CatCommand implementation..."
cat commands/spigot-platform/src/test/java/net/apartium/cocoabeans/commands/spigot/parsers/CatCommand.java
# Also search for any Command annotations to ensure we catch all command methods
echo -e "\nSearching for @Command annotations in CatCommand..."
rg "@Command" commands/spigot-platform/src/test/java/net/apartium/cocoabeans/commands/spigot/parsers/CatCommand.java -A 2
Length of output: 998
commands/src/test/java/net/apartium/cocoabeans/commands/parsers/compound/PositionParser.java (2)
20-24: LGTM! Clear and straightforward implementation
The parser variant is well-defined with appropriate annotations and a clean implementation.
1-7: Verify if this class should be in src/main instead of src/test
This appears to be a concrete implementation of a position parser, but it's located in the test directory. If this is meant to be used in production code, consider moving it to src/main.
commands/spigot-platform/src/test/java/net/apartium/cocoabeans/commands/spigot/parsers/MeowParser.java (2)
1-39: Verify the Meow class implementation
The parser depends on the Meow class, but we should verify its implementation to ensure compatibility.
✅ Verification successful
MeowParser implementation is correctly aligned with Meow class
The MeowParser implementation is properly aligned with the Meow record class:
- The parser's serialize method signature matches the Meow record's components (String cat, int age, Gender gender)
- The Gender enum is correctly defined with MALE, FEMALE, OTHER values
- The getGenders() method in MeowParser correctly maps the enum values
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash
# Search for Meow class definition
ast-grep --pattern 'class Meow {
$$$
}'
# Search for Meow.Gender enum definition
ast-grep --pattern 'enum Gender {
$$$
}'
Length of output: 88
Script:
#!/bin/bash
# Search for Meow class using ripgrep to find any file containing it
rg -l "class\s+Meow"
# Search for files containing Gender enum
rg -l "enum\s+Gender"
# If files are found, we'll examine their content in the next iteration
Length of output: 354
Script:
#!/bin/bash
# Let's examine the Meow class implementation
cat commands/spigot-platform/src/test/java/net/apartium/cocoabeans/commands/spigot/parsers/Meow.java
Length of output: 403
1-13: Consider moving to main source if this isn't a test class
This class is located in the test directory (src/test), but it appears to be a full implementation rather than a test class. If this is intended to be used in production code, consider moving it to the main source directory.
Writerside/cb.tree (1)
24-24: LGTM! Documentation structure is properly organized.
The new compound parser documentation topic is correctly placed under the Examples section, maintaining a logical documentation hierarchy.
Let's verify that the referenced markdown file exists:
✅ Verification successful
Documentation structure and referenced file verified successfully
The compound-parser.md file exists at the expected location in Writerside/topics/, confirming proper documentation setup.
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash
# Description: Verify the existence of the compound parser documentation file
# Expected: File should exist in the Writerside/topics directory
fd "compound-parser.md" --type f
Length of output: 69
commands/src/main/java/net/apartium/cocoabeans/commands/ArgumentMapper.java (1)
30-30: Verify implementations of ArgumentMapper interface
The change from RegisteredCommandVariant.Parameter[] to RegisteredVariant.Parameter[] is a breaking change that requires updates to all implementing classes.
Let's verify all implementations have been updated:
✅ Verification successful
All implementations correctly use the updated parameter type
The verification shows that:
- All three implementations of
ArgumentMapperhave been identified:SimpleArgumentMapperSimpleExceptionArgumentMapperSpigotExceptionArgumentMapper
- All occurrences of the parameter type are using the new
RegisteredVariant.Parameterformat - No legacy usages of
RegisteredCommandVariant.Parameterremain in the codebase
The breaking change has been properly propagated across all implementing classes and related code.
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash
# Description: Find all implementations of ArgumentMapper and verify they use the new type
# Find all Java files that might implement ArgumentMapper
ast-grep --pattern 'class $_ implements $$$ArgumentMapper$$$'
# Search for any remaining usage of RegisteredCommandVariant.Parameter
rg "RegisteredCommandVariant\.Parameter"
Length of output: 106
Script:
#!/bin/bash
# Let's try a different approach to find implementations and usages
# Find files containing "implements ArgumentMapper"
rg -l "implements.*ArgumentMapper"
# Find all Java files that might contain ArgumentMapper implementations
fd -e java -x grep -l "class.*implements.*ArgumentMapper" {}
# Search for any remaining usage of RegisteredCommandVariant (broader search)
rg "RegisteredCommandVariant"
# Search for current usage of RegisteredVariant.Parameter
rg "RegisteredVariant\.Parameter"
Length of output: 3654
commands/src/main/java/net/apartium/cocoabeans/commands/CommandNode.java (1)
18-18: Verify the impact of GenericNode extension
The change to extend GenericNode is architecturally sound and aligns with the PR's objective of supporting compound parsing. However, we should verify the impact on existing CommandNode implementations.
✅ Verification successful
Let me try a different search strategy to verify the impact of GenericNode extension.
Extension of GenericNode has no breaking impact
The extension of CommandNode to include GenericNode is safe and non-breaking because:
GenericNodeis a marker interface with no methods or constants- It's a newly introduced interface (since version 0.0.37) specifically designed for command-related components
- The change is purely additive and doesn't modify existing
CommandNodecontract
The interface serves as an architectural marker to group command-related components (nodes, compound parsers, flags) under a common type hierarchy, which aligns well with the PR's compound parser objectives.
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash
# Description: Find all implementations of CommandNode to assess impact
# Expected results: List of classes that need to be updated
# Find direct implementations of CommandNode
echo "Direct CommandNode implementations:"
ast-grep --pattern 'class $_ implements $$$CommandNode$$$'
# Find classes that extend other classes implementing CommandNode
echo -e "\nIndirect CommandNode implementations:"
ast-grep --pattern 'class $_ extends $_ implements $$$CommandNode$$$'
Length of output: 304
Script:
#!/bin/bash
# Search for any file containing CommandNode reference
echo "Files referencing CommandNode:"
rg -l "CommandNode"
# Search for GenericNode interface definition to understand the contract
echo -e "\nGenericNode interface definition:"
fd -e java -e kt -e scala GenericNode --exec cat {}
Length of output: 4916
commands/src/test/java/net/apartium/cocoabeans/commands/WallRequirementFactory.java (1)
18-18: LGTM! Verify type hierarchy for safety.
The parameter type change from CommandNode to GenericNode aligns with the broader refactoring across requirement factories.
Let's verify the type hierarchy to ensure type safety:
✅ Verification successful
Type hierarchy change is correct and consistent
The verification confirms that:
CommandNodeextendsGenericNode(commands/src/main/java/net/apartium/cocoabeans/commands/CommandNode.java)- All
RequirementFactoryimplementations consistently useGenericNodein theirgetRequirementmethod signatures - The change aligns with the interface definition in
RequirementFactory.java
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash
# Description: Verify the relationship between CommandNode and GenericNode
# Check if CommandNode extends/implements GenericNode
ast-grep --pattern 'interface CommandNode extends GenericNode'
# Check for other implementations to ensure consistency
ast-grep --pattern 'class $_ implements RequirementFactory {
$$$
getRequirement(GenericNode $_, $_) {
$$$
}
}'
Length of output: 187
Script:
#!/bin/bash
# Let's try a different approach to verify the type hierarchy and usage patterns
# Search for GenericNode definition and its relationships
rg -t java "interface GenericNode|class GenericNode" -A 5
# Search for CommandNode definition and its relationships
rg -t java "interface CommandNode|class CommandNode" -A 5
# Look for other RequirementFactory implementations to check parameter consistency
rg -t java "implements RequirementFactory" -A 3
# Search for getRequirement method signatures
rg -t java "getRequirement\s*\([^)]*\)"
Length of output: 6846
commands/src/test/java/net/apartium/cocoabeans/commands/multilayered/PermissionFactory.java (2)
4-4: LGTM: Import addition aligns with generic node type transition
The addition of GenericNode import is consistent with the broader refactoring effort to use more generic node types across the command framework.
13-13: LGTM: Method signature updated for generic node support
The parameter type change from CommandNode to GenericNode aligns with the architectural improvements mentioned in the PR objectives.
Let's verify consistent usage across the codebase:
✅ Verification successful
Based on the search results, I can now provide a definitive response:
Method signature change is consistent with codebase migration to GenericNode
The verification shows that:
- All requirement factory implementations consistently use
GenericNodein theirgetRequirementmethod signatures - The interface
CommandNodeextendsGenericNode, confirming the architectural relationship - No remaining usage of
CommandNodein requirement factory method signatures was found
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash
# Description: Verify consistent usage of GenericNode vs CommandNode in requirement factories
# Expected: All requirement factories should use GenericNode in their signatures
# Search for other requirement factory implementations
ast-grep --pattern 'class $_ implements RequirementFactory {
$$$
getRequirement($node, $_) {
$$$
}
}'
# Search for any remaining CommandNode usage in method signatures
rg -l "getRequirement\s*\(\s*CommandNode\s"
Length of output: 159
Script:
#!/bin/bash
# Let's try a different approach to verify the changes
# 1. Find all files containing RequirementFactory interface/class
rg -l "RequirementFactory"
# 2. Search for method signatures with GenericNode
rg "getRequirement.*GenericNode" -A 2
# 3. Search for any remaining CommandNode references
rg "CommandNode" -A 2
# 4. Find all implementations of RequirementFactory
ast-grep --pattern 'implements RequirementFactory'
Length of output: 39165
commands/src/test/java/net/apartium/cocoabeans/commands/parsers/compound/PositionParserTest.java (1)
1-11: LGTM! Class structure and imports are well-organized.
The test class is properly structured with appropriate imports and follows JUnit 5 conventions.
commands/src/main/java/net/apartium/cocoabeans/commands/requirements/argument/RangeArgumentRequirementFactory.java (1)
25-25: Verify GenericNode compatibility
The parameter type change from CommandNode to GenericNode looks good and aligns with the codebase-wide refactoring. However, let's verify that all callers have been updated.
✅ Verification successful
Let me check the test file to see how RangeArgumentRequirementFactory is being used.
Parameter type change from CommandNode to GenericNode is safe
The test file shows that RangeArgumentRequirementFactory is only used for testing and the test passes null values to getArgumentRequirement(), which means the parameter type change from CommandNode to GenericNode has no impact on existing functionality. The test verifies basic null handling behavior and doesn't rely on specific node type implementations.
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash
# Search for any remaining references to RangeArgumentRequirementFactory that might still use CommandNode
rg -l "RangeArgumentRequirementFactory" | xargs rg "CommandNode.*RangeArgumentRequirementFactory"
# Search for any test cases that need updating
fd -e java -e kt --full-path ".*[Tt]est.*" | xargs rg "RangeArgumentRequirementFactory"
Length of output: 748
Script:
#!/bin/bash
# Get the content of the test file to verify the usage
cat commands/src/test/java/net/apartium/cocoabeans/commands/GeneralCommandTest.java
Length of output: 29730
commands/src/main/java/net/apartium/cocoabeans/commands/AbstractCommandProcessingContext.java (1)
Line range hint 23-77: Overall class design looks good
The class demonstrates good practices:
- Proper encapsulation with final fields
- Clear separation of concerns
- Thread-safe immutable state (except for error handling)
- Comprehensive implementation of the
CommandProcessingContextinterface
commands/src/main/java/net/apartium/cocoabeans/commands/parsers/SourceParserFactory.java (2)
5-5: LGTM! Import changes are appropriate.
The addition of GenericNode import while retaining CommandNode is correct for the type migration.
28-28: Verify callers of getArgumentParser are updated.
The parameter type change from CommandNode to GenericNode looks correct, but we should ensure all callers have been updated.
✅ Verification successful
All callers have been updated to use GenericNode
Based on the search results:
- All implementations of
getArgumentParserin the codebase (SourceParserFactory, IntRangeParserFactory, WithParserFactory) useGenericNode - The interface
ParserFactorydefines the method withGenericNode - The only reference to
CommandNodewithgetArgumentParseris in documentation (Writerside/topics/Dynamic-annotation-parser.md) CommandNodenow extendsGenericNode, making the change type-safe
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash
# Search for any remaining references to getArgumentParser with CommandNode
ast-grep --pattern 'getArgumentParser($node: CommandNode, $_, $_)'
# Search for updated calls using GenericNode
ast-grep --pattern 'getArgumentParser($node: GenericNode, $_, $_)'
Length of output: 137
Script:
#!/bin/bash
# Search for method calls to getArgumentParser using ripgrep
rg "getArgumentParser\(" -A 2
# Search for files containing CommandNode references
rg "CommandNode" --type java
# Search for the implementation to understand usage
ast-grep --pattern 'getArgumentParser($$$) {
$$$
}'
Length of output: 14233
commands/spigot-platform/src/main/java/net/apartium/cocoabeans/commands/spigot/requirements/factory/PermissionFactory.java (2)
41-47: LGTM! Improved condition readability
The added parentheses in the condition check enhance readability and make the operator precedence explicit, while maintaining the same logical behavior.
27-31: LGTM! Method signature change aligns with framework updates
The change from CommandNode to GenericNode appears to be part of a broader refactoring effort to enhance type safety and flexibility in the command framework.
✅ Verification successful
Method signature change is consistent across the codebase
The verification confirms that all RequirementFactory implementations consistently use GenericNode in their getRequirement method signatures, including:
- WallRequirementFactory
- SenderLimitFactory
- WhitelistRequirementFactory
- InGameFactory
- PermissionFactory (both test and main implementations)
The change from CommandNode to GenericNode has been properly propagated across the entire codebase.
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash
# Search for other RequirementFactory implementations to verify consistent signature changes
ast-grep --pattern 'class $_ implements RequirementFactory {
$$$
public Requirement getRequirement($_, $_) {
$$$
}
}'
Length of output: 129
Script:
#!/bin/bash
# Let's try a different approach to find RequirementFactory implementations
# First, find files that might contain RequirementFactory
rg -l "RequirementFactory"
# Then search for getRequirement method signatures
rg "getRequirement\s*\([^)]*\)" -A 2
Length of output: 7865
commands/spigot-platform/src/test/java/net/apartium/cocoabeans/commands/spigot/game/commands/utils/InGameFactory.java (2)
4-4: LGTM: Import changes align with the generic node architecture.
The addition of GenericNode import supports the transition to a more flexible command processing structure.
17-17: Verify consistent implementation across RequirementFactory implementations.
The signature change from CommandNode to GenericNode is part of the broader enhancement to command handling.
✅ Verification successful
All RequirementFactory implementations consistently use GenericNode parameter
Based on the search results, all implementations of RequirementFactory consistently use the updated getRequirement(GenericNode node, Object obj) signature:
- WallRequirementFactory
- PermissionFactory (both test and main implementations)
- SenderLimitFactory
- WhitelistRequirementFactory
- InGameFactory
The signature change has been properly propagated across all implementations in the codebase.
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash
# Description: Verify that all RequirementFactory implementations use GenericNode
# Expected: All implementations should use the updated signature
# Search for RequirementFactory implementations
ast-grep --pattern 'class $_ implements RequirementFactory {
$$$
getRequirement($type node, $_) {
$$$
}
}'
Length of output: 118
Script:
#!/bin/bash
# Let's try a different approach to find RequirementFactory implementations and their method signatures
# First, find all files that might contain RequirementFactory
rg "RequirementFactory" -l
# Then look for the actual implementations with context
rg "implements.*RequirementFactory" -A 5
# Also search for getRequirement method definitions
rg "getRequirement\s*\([^)]*\)" -A 2
Length of output: 14189
commands/src/main/java/net/apartium/cocoabeans/commands/parsers/WithParserFactory.java (2)
85-118: Implementation looks solid and well-structured!
The method handles both WithParsers and WithParser annotations appropriately, with proper error handling and scope awareness. The transition from CommandNode to GenericNode maintains the same robust functionality.
3-4: Verify the relationship between CommandNode and GenericNode
Both CommandNode and GenericNode are imported. Let's verify their relationship and ensure this doesn't lead to confusion.
✅ Verification successful
Both imports are correct and necessary
CommandNode is an interface that extends GenericNode, making this a proper inheritance relationship. The import of both types in WithParserFactory.java is valid as it represents a transition to use the more generic parent type (GenericNode) while maintaining compatibility with the specific CommandNode interface.
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash
# Check the relationship between CommandNode and GenericNode
ast-grep --pattern 'class CommandNode {
$$$
}' -l java
ast-grep --pattern 'class GenericNode {
$$$
}' -l java
# Check if CommandNode extends/implements GenericNode
ast-grep --pattern 'class CommandNode $_ GenericNode {
$$$
}' -l java
Length of output: 191
Script:
#!/bin/bash
# Let's try a different approach to find these classes and their relationship
# First, let's find the files containing these class names
rg -l "class\s+CommandNode" --type java
rg -l "class\s+GenericNode" --type java
# Then let's look at the actual class definitions with context
rg "class\s+CommandNode" --type java -B 2 -A 5
rg "class\s+GenericNode" --type java -B 2 -A 5
# Also search for any usage patterns that might indicate their relationship
rg "extends\s+GenericNode|implements\s+GenericNode" --type java
Length of output: 537
commands/spigot-platform/src/main/java/net/apartium/cocoabeans/commands/spigot/requirements/factory/WhitelistRequirementFactory.java (2)
Line range hint 34-94: LGTM: Security implementation remains robust
The security implementation maintains proper checks:
- Null sender validation
- Type checking for Player and ConsoleCommandSender
- UUID-based whitelist verification
- Proper error handling with custom exceptions
The change to GenericNode doesn't impact the security model.
34-34: Verify all callers handle GenericNode correctly
The change from CommandNode to GenericNode is part of a broader refactoring. While the change itself looks correct, we should ensure all callers have been properly updated.
✅ Verification successful
Change from CommandNode to GenericNode is consistent with the codebase
The change is part of a broader refactoring where:
- The
RequirementFactoryinterface explicitly defines the contract usingGenericNode - Other requirement factories (PermissionFactory, SenderLimitFactory) have undergone the same change
- The change maintains type safety as
GenericNodeis properly imported and used across the requirements package - The
@CommandRequirementTypeannotation system ensures proper factory binding
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash
# Search for any remaining references to WhitelistRequirementFactory with CommandNode
rg -l "WhitelistRequirementFactory.*CommandNode"
# Search for all current usages of WhitelistRequirementFactory
rg -A 3 "new WhitelistRequirementFactory\(\)"
Length of output: 98
Script:
#!/bin/bash
# Let's try a broader search to find any references to WhitelistRequirementFactory
rg -A 5 "WhitelistRequirementFactory"
# Also search for implementations of getRequirement method with GenericNode
ast-grep --pattern 'getRequirement($node, $obj) { $$$ }'
# Search for any class that extends or implements WhitelistRequirementFactory
rg -A 3 "extends.*WhitelistRequirementFactory|implements.*WhitelistRequirementFactory"
Length of output: 2945
Script:
#!/bin/bash
# Let's look for the RequirementFactory interface to understand the contract
rg -A 5 "interface RequirementFactory"
# Search for any usage of @CommandRequirementType to find other similar factories
rg -A 3 "@CommandRequirementType"
# Look for any references to CommandNode in the requirements package
rg -A 3 "CommandNode" commands/spigot-platform/src/main/java/net/apartium/cocoabeans/commands/spigot/requirements/
Length of output: 6169
commands/src/main/java/net/apartium/cocoabeans/commands/requirements/ArgumentRequirementFactory.java (3)
14-14: LGTM! Import changes align with GenericNode migration
The replacement of CommandNode with GenericNode import is consistent with the broader refactoring effort mentioned in the PR objectives.
21-24: LGTM! Appropriate utility imports added
The new utility imports support the implementation of the createArgumentRequirements method.
112-112: LGTM! Verify GenericNode usage across implementations
The signature change from CommandNode to GenericNode is appropriate and aligns with the refactoring effort.
✅ Verification successful
Let me gather more information about the implementations of ArgumentRequirementFactory to ensure the change is properly reflected.
Let me check the actual implementation of RangeArgumentRequirementFactory to verify it's using GenericNode correctly.
Let me try one final verification with a different approach to check the implementation.
Change from CommandNode to GenericNode is properly implemented
The implementation in RangeArgumentRequirementFactory correctly uses the GenericNode parameter type, and there are no remaining usages of CommandNode in the argument requirement implementations. The change is consistent across the codebase.
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash
# Description: Verify consistent usage of GenericNode across implementations
# Look for any remaining CommandNode usage in implementations
# Search for implementations still using CommandNode
ast-grep --pattern 'class $_ implements ArgumentRequirementFactory {
$$$
ArgumentRequirement getArgumentRequirement($node, $_) {
$$$
}
}'
# Search for direct CommandNode references
rg "CommandNode" --type java
Length of output: 11192
Script:
#!/bin/bash
# Search for implementations of ArgumentRequirementFactory
ast-grep --pattern 'class $_ implements ArgumentRequirementFactory {
$$$
}'
# Search for files containing ArgumentRequirementFactory
rg "ArgumentRequirementFactory" --type java
Length of output: 4716
Script:
#!/bin/bash
# Get the implementation of getArgumentRequirement in RangeArgumentRequirementFactory
ast-grep --pattern 'class RangeArgumentRequirementFactory implements ArgumentRequirementFactory {
$$$
getArgumentRequirement($_, $_) {
$$$
}
$$$
}'
Length of output: 161
Script:
#!/bin/bash
# Get the content of RangeArgumentRequirementFactory.java
cat commands/src/main/java/net/apartium/cocoabeans/commands/requirements/argument/RangeArgumentRequirementFactory.java
Length of output: 2717
commands/spigot-platform/src/main/java/net/apartium/cocoabeans/commands/spigot/requirements/factory/SenderLimitFactory.java (2)
15-15: LGTM: Import addition aligns with the type hierarchy changes
The addition of the GenericNode import is consistent with the broader refactoring effort to enhance type flexibility in the command framework.
34-34: LGTM: Method signature change enhances type flexibility
The parameter type change from CommandNode to GenericNode aligns with the framework's evolution towards more flexible type handling.
Let's verify consistent implementation across other requirement factories:
✅ Verification successful
Method signature change is consistently implemented across requirement factories
The verification shows that all requirement factory implementations consistently use GenericNode in their getRequirement method signatures:
PermissionFactory:getRequirement(GenericNode node, Object obj)WallRequirementFactory:getRequirement(GenericNode commandNode, Object obj)InGameFactory:getRequirement(GenericNode node, Object obj)WhitelistRequirementFactory:getRequirement(GenericNode node, Object obj)SenderLimitFactory:getRequirement(GenericNode node, Object obj)
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash
# Description: Check if all requirement factories consistently use GenericNode
# Search for RequirementFactory implementations and their getRequirement signatures
ast-grep --pattern 'class $_ implements RequirementFactory {
$$$
public Requirement getRequirement($type $_, $_) {
$$$
}
}'
Length of output: 135
Script:
#!/bin/bash
# Let's try a different approach to find requirement factory implementations
# First, find all files that might contain requirement factories
fd -e java | xargs rg -l "RequirementFactory"
# Then, let's examine the content of these files to see the method signatures
fd -e java | xargs rg "getRequirement.*\(" -A 1
Length of output: 7074
commands/src/main/java/net/apartium/cocoabeans/commands/parsers/factory/IntRangeParserFactory.java (2)
16-16: LGTM: Import addition aligns with architecture changes
The addition of GenericNode import supports the transition to a more flexible command framework.
37-37: Verify consistent usage of GenericNode parameter type
The parameter type change from CommandNode to GenericNode looks good and aligns with the architectural improvements. Let's verify this pattern is consistently applied across other parser factories.
✅ Verification successful
Parameter type change to GenericNode is consistently implemented
The verification shows that all parser factory implementations consistently use GenericNode as the parameter type in their getArgumentParser method:
WithParserFactory:getArgumentParser(GenericNode node, ...)SourceParserFactory:getArgumentParser(GenericNode node, ...)IntRangeParserFactory:getArgumentParser(GenericNode node, ...)ParserFactoryinterface:getArgumentParser(GenericNode node, ...)
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash
# Description: Check for consistency in parser factory implementations
# Verify all parser factories use GenericNode in their getArgumentParser method
# Search for parser factory implementations
ast-grep --pattern 'class $_ implements ParserFactory {
$$$
public $_ getArgumentParser($type $_, $_, $_) {
$$$
}
$$$
}'
Length of output: 134
Script:
#!/bin/bash
# Let's try a different approach to find parser factory implementations
# First, find all files that might contain parser factories
rg -l "ParserFactory" --type java
# Then, let's examine the method signatures in these files
rg "getArgumentParser\(" --type java -A 1
Length of output: 3120
commands/src/main/java/net/apartium/cocoabeans/commands/CommandBranchProcessor.java (1)
120-120: Verify impact of argument ordering change
The change from add(0, value.value()) to add(value.value()) reverses the order in which parsed arguments are stored in the list. While this might be intentional to support the new Compound Parser feature, we should verify that this change doesn't break any existing command handlers that might depend on argument order.
Let's verify any potential order dependencies:
Consider adding a comment in the code documenting this intentional change in argument ordering, especially if it's related to supporting the new Compound Parser feature. This will help prevent accidental reversals in future modifications.
✅ Verification successful
The search results reveal important insights. Let's verify one more aspect to make a conclusive decision:
This will help us understand if there are any explicit order dependencies in the argument processing logic and any documented expectations about argument ordering.
Change in argument ordering is safe
The verification shows that the argument order change is safe because:
SimpleArgumentMapperaccesses arguments by specific indices that are determined during mapping, not by relying on insertion orderCommandOptionconsistently usesadd(0, ...)for its own operations, maintaining its specific ordering needs- No command handlers were found that depend on the overall argument order
- No documentation or code comments indicate order-sensitive processing of parsed arguments
The change from add(0, value.value()) to add(value.value()) in CommandBranchProcessor appears to be an intentional simplification that doesn't impact existing functionality.
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash
# Description: Search for any code that might depend on the order of parsed arguments
# Look for direct access to parsed arguments list
rg -A 5 "parsedArgs\(\)" commands/
# Look for potential command handlers that might process these arguments
ast-grep --pattern 'class $_ implements CommandHandler {
$$$
handle($$$) {
$$$
parsedArgs
$$$
}
}'
Length of output: 4738
Script:
#!/bin/bash
# Look for any code that processes lists from parsedArgs to understand order dependencies
rg -A 10 "get\(.*\)\.get\([0-9]" commands/
# Look for any documentation or comments about argument ordering
rg -i "order|sequence" commands/
Length of output: 1686
commands/src/main/java/net/apartium/cocoabeans/commands/SimpleArgumentMapper.java (2)
134-137: LGTM! Type change is consistent
The parameter type change is consistent with the broader refactoring and maintains all existing functionality and error handling.
Line range hint 95-104: Verify type compatibility across command framework
The parameter type change from RegisteredCommandVariant.Parameter[] to RegisteredVariant.Parameter[] looks good. However, we should ensure all callers have been updated accordingly.
✅ Verification successful
Type change from RegisteredCommandVariant.Parameter to RegisteredVariant.Parameter is consistently applied
The verification shows that all references to the parameter type are using the new RegisteredVariant.Parameter class consistently across the codebase. No instances of the old RegisteredCommandVariant.Parameter type remain, and the new type is properly used in all relevant files:
SimpleArgumentMapper.javaRegisteredCommand.javaArgumentMapper.javaCompoundParser.java
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash
# Description: Verify all usages of RegisteredCommandVariant.Parameter have been updated
# to use RegisteredVariant.Parameter
# Search for any remaining references to RegisteredCommandVariant.Parameter
rg "RegisteredCommandVariant\.Parameter"
# Search for updated references to RegisteredVariant.Parameter
rg "RegisteredVariant\.Parameter"
Length of output: 2182
commands/src/main/java/net/apartium/cocoabeans/commands/CommandOption.java (1)
38-38: LGTM: Consistent usage of renamed field
The empty checks on registeredVariants are used consistently in both handle() and handleOptional() methods, maintaining the original control flow logic.
Also applies to: 183-183
commands/src/testFixtures/java/net/apartium/cocoabeans/commands/parsers/ParserAssertions.java (1)
328-367: LGTM! Well-structured tab completion assertion implementation
The implementation:
- Handles all edge cases appropriately
- Maintains consistent error reporting with other assertion methods
- Properly validates both completion results and cursor position
- Uses clear and maintainable control flow
commands/src/main/java/net/apartium/cocoabeans/commands/parsers/CompoundParserOption.java (1)
18-53: Well-implemented parse method with robust parsing logic
The parse method effectively processes command arguments and utilizes the Optional class to handle possible absence of values gracefully. The iteration over argumentTypeHandlerMap and the handling of parsing results are well-structured.
commands/src/main/java/net/apartium/cocoabeans/commands/requirements/RequirementFactory.java (2)
50-50:
Ensure project compatibility with Java 9 or higher
The use of private static methods in interfaces (private static Requirement getRequirement()) is a feature introduced in Java 9. Confirm that the project targets Java 9 or higher to avoid compatibility issues.
Run the following script to verify the project's Java version settings:
110-110:
Potential breaking change due to method signature update
Changing the parameter type from CommandNode to GenericNode in the getRequirement method may break existing implementations of the RequirementFactory interface. Implementing classes will need to update their method signatures to match the new definition.
Ensure all implementing classes are updated accordingly. Run the following script to identify implementations of RequirementFactory that need attention:
commands/src/main/java/net/apartium/cocoabeans/commands/RegisteredCommand.java (6)
32-33: Static import of REGISTERED_VARIANT_COMPARATOR is appropriate
The addition of the static import for REGISTERED_VARIANT_COMPARATOR enhances readability and simplifies its usage throughout the class.
72-72: Proper construction of requirement sets using fallbackHandle annotations
The inclusion of RequirementFactory.createRequirementSet with fallbackHandle.getAnnotations() correctly extends the existing requirement set, ensuring that fallback requirements are accounted for.
79-82: Correct merging of class parsers into argumentTypeHandlerMap
The use of CollectionHelpers.mergeInto to combine class parsers retrieved by ParserFactory.findClassParsers into argumentTypeHandlerMap is appropriate and maintains parser consistency.
85-88: Inclusion of command manager's argument parsers is well-handled
Merging commandManager.argumentTypeHandlerMap into argumentTypeHandlerMap ensures that all global argument parsers are available, which is essential for comprehensive parsing.
225-225: Consistent creation of RegisteredVariant.Parameter arrays
The use of RegisteredVariant.Parameter.of method to create parameter arrays maintains consistency and leverages factory methods for cleaner code.
Also applies to: 271-271
230-237: Appropriate use of REGISTERED_VARIANT_COMPARATOR for sorting variants
Utilizing REGISTERED_VARIANT_COMPARATOR within CollectionHelpers.addElementSorted ensures that command variants are sorted correctly according to their priority.
Also applies to: 275-282
...pigot-platform/src/test/java/net/apartium/cocoabeans/commands/spigot/parsers/CatCommand.java
Show resolved
Hide resolved
...pigot-platform/src/test/java/net/apartium/cocoabeans/commands/spigot/parsers/CatCommand.java
Show resolved
Hide resolved
...-platform/src/test/java/net/apartium/cocoabeans/commands/spigot/parsers/MeowCommandTest.java
Outdated
Show resolved
Hide resolved
commands/src/test/java/net/apartium/cocoabeans/commands/parsers/compound/PositionParser.java
Show resolved
Hide resolved
...pigot-platform/src/test/java/net/apartium/cocoabeans/commands/spigot/parsers/MeowParser.java
Show resolved
Hide resolved
commands/src/main/java/net/apartium/cocoabeans/commands/requirements/RequirementFactory.java
Show resolved
Hide resolved
commands/src/main/java/net/apartium/cocoabeans/commands/parsers/ParserFactory.java
Show resolved
Hide resolved
commands/src/main/java/net/apartium/cocoabeans/commands/parsers/CompoundParser.java
Outdated
Show resolved
Hide resolved
commands/src/main/java/net/apartium/cocoabeans/commands/parsers/CompoundParser.java
Show resolved
Hide resolved
commands/src/main/java/net/apartium/cocoabeans/commands/parsers/CompoundParser.java
Outdated
Show resolved
Hide resolved
Closes #122
There was a problem hiding this comment.
Actionable comments posted: 4
🧹 Outside diff range and nitpick comments (9)
commands/spigot-platform/src/main/java/net/apartium/cocoabeans/commands/spigot/requirements/factory/PermissionFactory.java (2)
32-32: Address TODO comment regarding inversionThere's an unaddressed TODO comment about inversion. Consider implementing the inversion logic or documenting why it's not needed.
Would you like me to help implement the inversion logic or create a GitHub issue to track this?
39-45: Consider extracting duplicated error messageThe error message "You don't have permission to execute this command" is duplicated. Consider extracting it to a constant.
private record PermissionImpl(Permission permission, String permissionAsString, boolean invert) implements Requirement { + private static final String NO_PERMISSION_MESSAGE = "You don't have permission to execute this command"; @Override public RequirementResult meetsRequirement(RequirementEvaluationContext context) { // ... if ((sender.getSender() == null || !(sender.getSender() instanceof CommandSender commandSender))) { return RequirementResult.error(new UnmetPermissionResponse( this, context, - "You don't have permission to execute this command" + NO_PERMISSION_MESSAGE )); } if (!commandSender.hasPermission(permissionAsString)) return RequirementResult.error(new UnmetPermissionResponse( this, context, - "You don't have permission to execute this command" + NO_PERMISSION_MESSAGE ));Also applies to: 48-52
commands/src/main/java/net/apartium/cocoabeans/commands/SimpleCommandProcessingContext.java (2)
Line range hint
22-38: Consider adding @nullable annotation for error fieldThe class structure looks good with proper immutability. Consider adding
@Nullableannotation to theerrorfield to make nullability explicit in the API.- private BadCommandResponse error = null; + @Nullable + private BadCommandResponse error = null;
99-122: Consider improving error handling mechanismThe current error handling implementation could be enhanced in several ways:
- Consider returning
Optional<BadCommandResponse>fromgetReport()to make nullability explicit in the API- Consider adding thread safety if the context might be used across threads
- Consider adding error state change listeners for better integration with monitoring systems
- public BadCommandResponse getReport() { + public Optional<BadCommandResponse> getReport() { - return error; + return Optional.ofNullable(error); }commands/src/main/java/net/apartium/cocoabeans/commands/requirements/ArgumentRequirementFactory.java (1)
27-62: Consider returning List instead of array.The implementation is solid with proper null checks and error handling. However, there are a few suggestions for improvement:
- Consider returning
List<ArgumentRequirement>instead of array to provide more flexibility to callers and avoid array creation.- The nested Optional usage could be simplified.
Here's a suggested refactor:
- static ArgumentRequirement[] createArgumentRequirements(GenericNode node, Annotation[] annotations, Map<Class<? extends ArgumentRequirementFactory>, ArgumentRequirementFactory> argumentRequirementFactories) { + static List<ArgumentRequirement> createArgumentRequirements(GenericNode node, Annotation[] annotations, Map<Class<? extends ArgumentRequirementFactory>, ArgumentRequirementFactory> argumentRequirementFactories) { if (annotations == null) - return new ArgumentRequirement[0]; + return List.of(); List<ArgumentRequirement> result = new ArrayList<>(); for (Annotation annotation : annotations) { Class<? extends ArgumentRequirementFactory> argumentRequirementType = ArgumentRequirementFactory.getArgumentRequirementFactoryClass(annotation); if (argumentRequirementType == null) continue; - ArgumentRequirement argumentRequirement = Optional.ofNullable(argumentRequirementFactories.computeIfAbsent( - argumentRequirementType, - clazz -> ArgumentRequirementFactory.createFromAnnotation(annotation) - )) - .map(factory -> factory.getArgumentRequirement(node, annotation)) - .orElse(null); + ArgumentRequirementFactory factory = argumentRequirementFactories.computeIfAbsent( + argumentRequirementType, + clazz -> ArgumentRequirementFactory.createFromAnnotation(annotation) + ); + if (factory == null) continue; + + ArgumentRequirement argumentRequirement = factory.getArgumentRequirement(node, annotation); if (argumentRequirement == null) continue; result.add(argumentRequirement); } - return result.toArray(new ArgumentRequirement[0]); + return result; }commands/src/main/java/net/apartium/cocoabeans/commands/parsers/CompoundParser.java (4)
36-42: Complete the constructor documentationThe constructor's documentation is incomplete. Add parameter descriptions and explain the purpose of each parameter.
Add descriptions for:
argumentMapper: Purpose and usagecommandLexer: Role in command processing- Impact of
priorityon parser behavior
26-28: Consider using immutable maps for factoriesThe factory maps are cleared after initialization and should be immutable afterward. Consider using
ImmutableMaporCollections.unmodifiableMap()after population.- private final Map<Class<? extends RequirementFactory>, RequirementFactory> requirementFactories = new HashMap<>(); - private final Map<Class<? extends ParserFactory>, ParserFactory> parserFactories = new HashMap<>(); - private final Map<Class<? extends ArgumentRequirementFactory>, ArgumentRequirementFactory> argumentRequirementFactories = new HashMap<>(); + private final Map<Class<? extends RequirementFactory>, RequirementFactory> requirementFactories = Collections.synchronizedMap(new HashMap<>()); + private final Map<Class<? extends ParserFactory>, ParserFactory> parserFactories = Collections.synchronizedMap(new HashMap<>()); + private final Map<Class<? extends ArgumentRequirementFactory>, ArgumentRequirementFactory> argumentRequirementFactories = Collections.synchronizedMap(new HashMap<>());
98-100: Enhance error messages for better debuggingThe error messages for invalid tokens could be more descriptive to help with debugging.
- throw new IllegalArgumentException("Parser variant cannot be empty"); + throw new IllegalArgumentException("Parser variant cannot be empty. Variant: " + parserVariant.value()); - throw new UnknownTokenException(token); + throw new UnknownTokenException("Unexpected token type: " + token.getClass().getSimpleName() + ", value: " + token);Also applies to: 119-119
188-204: Consider using Optional for parameter list return valueInstead of returning null for failed requirements, consider using Optional to make the failure case more explicit and force handling of the null case.
- private List<Object> getParameters(RegisteredVariant registeredVariant, Sender sender, ArgumentContext context) { + private Optional<List<Object>> getParameters(RegisteredVariant registeredVariant, Sender sender, ArgumentContext context) { List<Object> parameters = new ArrayList<>(registeredVariant.argumentIndexList().stream() .<Object>map((argumentIndex -> argumentIndex.get(context))) .toList()); parameters.add(0, registeredVariant.node()); for (int i = 0; i < registeredVariant.parameters().length; i++) { Object obj = parameters.get(i + 1); // first element is class instance for (ArgumentRequirement argumentRequirement : registeredVariant.parameters()[i].argumentRequirements()) { if (!argumentRequirement.meetsRequirement(sender, null, obj)) - return null; + return Optional.empty(); } } - return parameters; + return Optional.of(parameters); }
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
📒 Files selected for processing (9)
CHANGELOG.md(2 hunks)commands/spigot-platform/src/main/java/net/apartium/cocoabeans/commands/spigot/requirements/factory/PermissionFactory.java(2 hunks)commands/spigot-platform/src/test/java/net/apartium/cocoabeans/commands/spigot/parsers/MeowCommandTest.java(1 hunks)commands/src/main/java/net/apartium/cocoabeans/commands/CommandOption.java(8 hunks)commands/src/main/java/net/apartium/cocoabeans/commands/SimpleCommandProcessingContext.java(2 hunks)commands/src/main/java/net/apartium/cocoabeans/commands/parsers/CompoundParser.java(1 hunks)commands/src/main/java/net/apartium/cocoabeans/commands/parsers/CompoundParserOption.java(1 hunks)commands/src/main/java/net/apartium/cocoabeans/commands/requirements/ArgumentRequirementFactory.java(3 hunks)commands/src/test/java/net/apartium/cocoabeans/commands/GeneralCommandTest.java(2 hunks)
🚧 Files skipped from review as they are similar to previous changes (4)
- CHANGELOG.md
- commands/spigot-platform/src/test/java/net/apartium/cocoabeans/commands/spigot/parsers/MeowCommandTest.java
- commands/src/main/java/net/apartium/cocoabeans/commands/CommandOption.java
- commands/src/main/java/net/apartium/cocoabeans/commands/parsers/CompoundParserOption.java
🧰 Additional context used
📓 Learnings (1)
commands/src/main/java/net/apartium/cocoabeans/commands/requirements/ArgumentRequirementFactory.java (1)
Learnt from: ikfir
PR: PoweredByApartium/cocoa-beans#214
File: commands/src/main/java/net/apartium/cocoabeans/commands/requirements/ArgumentRequirementFactory.java:30-43
Timestamp: 2024-11-22T20:02:07.282Z
Learning: The `CommandManager` class already implements caching mechanisms, so additional caching within factory methods like `ArgumentRequirementFactory.create()` is unnecessary.
🔇 Additional comments (10)
commands/spigot-platform/src/main/java/net/apartium/cocoabeans/commands/spigot/requirements/factory/PermissionFactory.java (3)
13-13: LGTM! Import changes align with method signature update
The switch to importing GenericNode instead of CommandNode correctly reflects the updated method signature.
25-30: LGTM! Clean implementation of requirement factory method
The change to use GenericNode instead of CommandNode improves the flexibility of the factory while maintaining type safety through pattern matching.
38-45: LGTM! Improved null check safety
The restructured condition properly checks for null before attempting type casting, improving code safety.
commands/src/main/java/net/apartium/cocoabeans/commands/SimpleCommandProcessingContext.java (2)
53-98: LGTM! Well-implemented interface methods
The core interface methods are properly implemented with clear documentation and good practices:
- Immutable fields are returned directly
- Methods are focused and simple
- Documentation is clear and helpful
Line range hint 22-122: Verify integration with Compound Parser feature
Since this PR introduces the Compound Parser feature, we should verify that this command processing context properly supports compound parsing operations.
✅ Verification successful
Let me gather more specific information about how CommandProcessingContext is used in the compound parser implementation.
Based on the code analysis, I can now provide the final response:
SimpleCommandProcessingContext implementation properly supports Compound Parser feature
The implementation correctly supports the Compound Parser feature by providing all necessary context information:
- It's used extensively in the CompoundParser's parse and tabCompletion methods
- The context properly exposes all required data (sender, label, args, index) needed for compound parsing operations
- The error reporting mechanism is properly utilized by CompoundParser when no matching variants are found
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash
# Search for Compound Parser related classes and their interaction with CommandProcessingContext
echo "Searching for Compound Parser implementations..."
rg -l "CompoundParser|LocationParser" --type java
echo "Checking for CommandProcessingContext usage in parser implementations..."
ast-grep --pattern 'class $_ implements Parser {
$$$
CommandProcessingContext
$$$
}'
Length of output: 1374
Script:
#!/bin/bash
# Check CompoundParser implementation for CommandProcessingContext usage
echo "Checking CompoundParser.java content:"
cat commands/src/main/java/net/apartium/cocoabeans/commands/parsers/CompoundParser.java
echo -e "\nChecking CompoundParserBranchProcessor.java content:"
cat commands/src/main/java/net/apartium/cocoabeans/commands/parsers/CompoundParserBranchProcessor.java
echo -e "\nChecking for any test cases involving SimpleCommandProcessingContext:"
rg "SimpleCommandProcessingContext" --type java -C 3
Length of output: 24002
commands/src/main/java/net/apartium/cocoabeans/commands/requirements/ArgumentRequirementFactory.java (2)
13-13: LGTM! Import changes align with the interface updates.
The switch from CommandNode to GenericNode and addition of collection imports support the interface's enhanced functionality.
Also applies to: 20-22
84-84: LGTM! Method signature updates improve the design.
The changes enhance the interface by:
- Removing unnecessary CommandManager parameter, as caching is already handled at that level
- Using GenericNode for better abstraction and flexibility
Also applies to: 111-111
commands/src/main/java/net/apartium/cocoabeans/commands/parsers/CompoundParser.java (2)
261-268: LGTM! Well-structured record definition
The ParserResult record is well-designed with clear and purposeful fields.
227-232:
Improve exception handling specificity
Catching Throwable is too broad and can mask serious issues. Consider catching specific exceptions that can be thrown by invokeWithArguments.
try {
output = (T) registeredVariant.method().invokeWithArguments(parameters);
- } catch (Throwable e) {
+ } catch (RuntimeException | Error e) {
+ throw e; // rethrow critical errors
+ } catch (Exception e) {
Dispensers.dispense(e);
- return Optional.empty(); // never going to reach this place
+ return Optional.empty();
}Likely invalid or redundant comment.
commands/src/test/java/net/apartium/cocoabeans/commands/GeneralCommandTest.java (1)
348-359: Consider using JMH for proper performance benchmarking
The implementation has several limitations that were previously identified. Please refer to the earlier review comment for detailed recommendations on using JMH and proper statistical analysis.
commands/src/main/java/net/apartium/cocoabeans/commands/SimpleCommandProcessingContext.java
Show resolved
Hide resolved
commands/src/main/java/net/apartium/cocoabeans/commands/parsers/CompoundParser.java
Show resolved
Hide resolved
commands/src/main/java/net/apartium/cocoabeans/commands/parsers/CompoundParser.java
Show resolved
Hide resolved
commands/src/test/java/net/apartium/cocoabeans/commands/GeneralCommandTest.java
Show resolved
Hide resolved
Closes #122
There was a problem hiding this comment.
Actionable comments posted: 1
🧹 Outside diff range and nitpick comments (5)
commands/spigot-platform/src/main/java/net/apartium/cocoabeans/commands/spigot/requirements/factory/PermissionFactory.java (3)
41-47: Consider extracting duplicate error message to a constantThe error message "You don't have permission to execute this command" is duplicated. Consider extracting it to a private static final constant for better maintainability.
+ private static final String NO_PERMISSION_MESSAGE = "You don't have permission to execute this command"; - "You don't have permission to execute this command" + NO_PERMISSION_MESSAGEAlso applies to: 50-54
62-63: Maintain consistent parentheses style with meetsRequirement methodFor consistency with the main method, consider using the same parentheses style:
- if (sender.getSender() == null || !(sender.getSender() instanceof CommandSender commandSender)) + if ((sender.getSender() == null || !(sender.getSender() instanceof CommandSender commandSender)))
Line range hint
22-89: Well-structured implementation with good separation of concernsThe overall architecture demonstrates good practices:
- Clean factory pattern implementation
- Proper use of records for immutable data
- Clear separation between normal and inverted permission checking
- Consistent error handling approach
commands/src/main/java/net/apartium/cocoabeans/commands/requirements/RequirementFactory.java (1)
33-47: Consider adding input validation for requirementFactories parameter.While the method handles null annotations well, it should also validate the
requirementFactoriesparameter to prevent potentialNullPointerException.Consider adding this validation at the start of the method:
static RequirementSet createRequirementSet(GenericNode node, Annotation[] annotations, Map<Class<? extends RequirementFactory>, RequirementFactory> requirementFactories) { + Objects.requireNonNull(requirementFactories, "requirementFactories map cannot be null"); + Objects.requireNonNull(node, "node cannot be null"); + if (annotations == null) return new RequirementSet();commands/src/main/java/net/apartium/cocoabeans/commands/CommandManager.java (1)
197-214: Consider optimizing parameter processing and improving readability.The parameter processing logic could be optimized and made more readable.
Consider these improvements:
private boolean invoke(CommandContext context, Sender sender, RegisteredVariant registeredVariant) { - List<Object> parameters = new ArrayList<>(registeredVariant.argumentIndexList().stream() - .<Object>map((argumentIndex -> argumentIndex.get(context.toArgumentContext()))) - .toList()); + List<Object> parameters = new ArrayList<>(registeredVariant.argumentIndexList().size() + 1); + parameters.add(registeredVariant.node()); + for (var argumentIndex : registeredVariant.argumentIndexList()) { + parameters.add(argumentIndex.get(context.toArgumentContext())); + } - parameters.add(0, registeredVariant.node()); for (int i = 0; i < registeredVariant.parameters().length; i++) { Object obj = parameters.get(i + 1); // first element is class instance for (ArgumentRequirement argumentRequirement : registeredVariant.parameters()[i].argumentRequirements()) { if (!argumentRequirement.meetsRequirement(sender, context, obj)) return false; } }This change:
- Avoids unnecessary stream operations
- Pre-sizes the ArrayList
- Adds the node parameter directly instead of inserting at index 0
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
📒 Files selected for processing (13)
commands/spigot-platform/src/main/java/net/apartium/cocoabeans/commands/spigot/requirements/factory/PermissionFactory.java(4 hunks)commands/spigot-platform/src/test/java/net/apartium/cocoabeans/commands/spigot/parsers/MeowCommandTest.java(1 hunks)commands/src/main/java/net/apartium/cocoabeans/commands/CommandBranchProcessor.java(1 hunks)commands/src/main/java/net/apartium/cocoabeans/commands/CommandManager.java(3 hunks)commands/src/main/java/net/apartium/cocoabeans/commands/CommandOption.java(11 hunks)commands/src/main/java/net/apartium/cocoabeans/commands/RegisteredVariant.java(1 hunks)commands/src/main/java/net/apartium/cocoabeans/commands/parsers/CompoundParser.java(1 hunks)commands/src/main/java/net/apartium/cocoabeans/commands/parsers/CompoundParserBranchProcessor.java(1 hunks)commands/src/main/java/net/apartium/cocoabeans/commands/parsers/CompoundParserOption.java(1 hunks)commands/src/main/java/net/apartium/cocoabeans/commands/parsers/ParserVariant.java(1 hunks)commands/src/main/java/net/apartium/cocoabeans/commands/requirements/RequirementFactory.java(2 hunks)commands/src/test/java/net/apartium/cocoabeans/commands/multilayered/PermissionFactory.java(2 hunks)commands/src/test/java/net/apartium/cocoabeans/commands/parsers/compound/PositionParserTest.java(1 hunks)
✅ Files skipped from review due to trivial changes (1)
- commands/src/main/java/net/apartium/cocoabeans/commands/RegisteredVariant.java
🚧 Files skipped from review as they are similar to previous changes (9)
- commands/src/main/java/net/apartium/cocoabeans/commands/CommandBranchProcessor.java
- commands/src/test/java/net/apartium/cocoabeans/commands/multilayered/PermissionFactory.java
- commands/src/main/java/net/apartium/cocoabeans/commands/CommandOption.java
- commands/spigot-platform/src/test/java/net/apartium/cocoabeans/commands/spigot/parsers/MeowCommandTest.java
- commands/src/main/java/net/apartium/cocoabeans/commands/parsers/CompoundParserBranchProcessor.java
- commands/src/main/java/net/apartium/cocoabeans/commands/parsers/ParserVariant.java
- commands/src/test/java/net/apartium/cocoabeans/commands/parsers/compound/PositionParserTest.java
- commands/src/main/java/net/apartium/cocoabeans/commands/parsers/CompoundParserOption.java
- commands/src/main/java/net/apartium/cocoabeans/commands/parsers/CompoundParser.java
🔇 Additional comments (7)
commands/spigot-platform/src/main/java/net/apartium/cocoabeans/commands/spigot/requirements/factory/PermissionFactory.java (1)
Line range hint 25-29: LGTM! Good use of pattern matching and type safety
The change to use GenericNode makes the method more flexible while maintaining type safety through proper pattern matching.
commands/src/main/java/net/apartium/cocoabeans/commands/requirements/RequirementFactory.java (3)
13-13: LGTM: Import statements are appropriate.
The new imports support the added functionality and are properly organized.
Also applies to: 19-21
105-105: LGTM: Method signature update is well-documented.
The parameter type change from CommandNode to GenericNode properly supports the compound parser feature while maintaining a clean interface.
Also applies to: 110-110
56-59:
Fix potential logical error in computeIfAbsent lambda.
The lambda expression ignores the clazz parameter and unnecessarily recalculates the requirement factory class, which could lead to inconsistencies.
Apply this fix:
RequirementFactory requirementFactory = requirementFactories.computeIfAbsent(
requirementFactoryClass,
- clazz -> RequirementFactory.createFromAnnotation(annotation)
+ clazz -> {
+ try {
+ return clazz.getConstructor().newInstance();
+ } catch (InstantiationException | IllegalAccessException | InvocationTargetException | NoSuchMethodException e) {
+ throw new RuntimeException("Failed to instantiate requirement factory: " + clazz, e);
+ }
+ }
);commands/src/main/java/net/apartium/cocoabeans/commands/CommandManager.java (3)
92-93: LGTM! Good refactoring of null context handling.
The extraction of null context handling into a separate method improves code organization and maintainability.
150-155: LGTM! Clean implementation of fallback handling.
The method is well-focused with a single responsibility and clear implementation.
105-111: Verify the impact of RegisteredVariant type change.
The change from RegisteredCommandVariant to RegisteredVariant might affect other parts of the codebase.
✅ Verification successful
Type change from RegisteredCommandVariant to RegisteredVariant is consistent across the codebase
The search results show that RegisteredCommandVariant appears only in method names (getRegisteredCommandVariants()), while RegisteredVariant is the actual type used throughout the codebase. This indicates a consistent type usage where the class is named RegisteredVariant and some method names still contain the word "Command" as part of their identifier, which is a normal naming convention and doesn't indicate any inconsistency.
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash
# Search for any remaining references to RegisteredCommandVariant
rg "RegisteredCommandVariant" --type java
# Search for all usages of RegisteredVariant to ensure consistent usage
rg "RegisteredVariant" --type java
Length of output: 6079
commands/src/main/java/net/apartium/cocoabeans/commands/CommandManager.java
Show resolved
Hide resolved
Closes #122
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Outside diff range and nitpick comments (1)
commands/src/main/java/net/apartium/cocoabeans/commands/CommandManager.java (1)
197-206: Consider performance optimization for parameter mappingThe stream operation could be optimized by using a more direct approach:
-List<Object> parameters = new ArrayList<>(registeredVariant.argumentIndexList().stream() - .<Object>map((argumentIndex -> argumentIndex.get(context.toArgumentContext()))) - .toList()); +List<Object> parameters = new ArrayList<>(registeredVariant.argumentIndexList().size() + 1); +for (var argumentIndex : registeredVariant.argumentIndexList()) { + parameters.add(argumentIndex.get(context.toArgumentContext())); +}This avoids the overhead of stream creation and intermediate collections.
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
📒 Files selected for processing (1)
commands/src/main/java/net/apartium/cocoabeans/commands/CommandManager.java(3 hunks)
🔇 Additional comments (5)
commands/src/main/java/net/apartium/cocoabeans/commands/CommandManager.java (5)
92-93: LGTM: Clean separation of null context handling
The extraction of null context handling into a separate method improves code organization.
150-155: LGTM: Clean fallback handling extraction
Good separation of concerns. The method has a single responsibility and is well-focused.
214-214: LGTM: Proper method invocation
The method invocation with mapped parameters is correctly implemented.
120-148:
Critical: Revise fallback handling logic
The current implementation has several issues:
- Fallbacks are called even when requirements aren't met (line 132)
- The method handles multiple responsibilities (requirement checking, fallback handling, error handling)
Consider separating the concerns as suggested in the previous review:
-private boolean handleNullContext(Sender sender, String commandName, String[] args, RegisteredCommand registeredCommand) throws Exception {
+private boolean handleNullContext(Sender sender, String commandName, String[] args, RegisteredCommand registeredCommand) throws Exception {
+ BadCommandResponse response = checkRequirements(sender, commandName, args, registeredCommand);
+ if (response != null) {
+ return handleRequirementFailure(response, sender, commandName, args, registeredCommand);
+ }
+ return handleFallback(sender, commandName, args, registeredCommand);
+}
+
+private BadCommandResponse checkRequirements(Sender sender, String commandName, String[] args, RegisteredCommand registeredCommand) {
BadCommandResponse badCommandResponse = null;
for (RegisteredCommand.RegisteredCommandNode listener : registeredCommand.getCommands()) {
RequirementResult requirementResult = listener.requirements().meetsRequirements(new RequirementEvaluationContext(sender, commandName, args, 0));
if (requirementResult.hasError()) {
badCommandResponse = requirementResult.getError();
break;
}
}
+ return badCommandResponse;
+}
+
+private boolean handleRequirementFailure(BadCommandResponse response, Sender sender, String commandName, String[] args, RegisteredCommand registeredCommand) throws Exception {
+ if (handleError(null, sender, commandName, args, registeredCommand, response.getError())) {
+ return true;
+ }
+ response.throwError();
+ return false;
+}105-111: Verify exception handling completeness
The change from catching Throwable to Exception is more specific, but ensure all possible exceptions are properly handled.
✅ Verification successful
Based on the search results, I can now generate the final response. The codebase shows extensive exception handling patterns, and the change from Throwable to Exception appears appropriate as the exceptions thrown in the codebase are primarily of type Exception or its subclasses (RuntimeException, IllegalArgumentException, etc.).
Exception handling is appropriate and complete
The change from Throwable to Exception is safe as the codebase consistently uses Exception types:
- Most exceptions are
RuntimeExceptionfor operational errors IllegalArgumentExceptionfor validation failures- Other specific exceptions like
NoSuchElementException,UnsupportedOperationException
No critical system-level Throwable types (like OutOfMemoryError) are explicitly thrown in the command-related code paths, making Exception catch block sufficient.
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash
# Search for throw statements in command-related classes to verify exception types
rg --type java "throw new \w+Exception|throws \w+Exception" -B 2 -A 2
Length of output: 62724
Closes #122
There was a problem hiding this comment.
Actionable comments posted: 5
🧹 Outside diff range and nitpick comments (4)
commands/spigot-platform/src/test/java/net/apartium/cocoabeans/commands/spigot/requirements/PermissionCommandTest.java (2)
14-27: Add test cleanup and improve field encapsulation.Consider the following improvements:
- Add cleanup of permissions between tests to prevent test interference
- Mark the player field as private final
- Add class-level documentation explaining the test purpose
+/** + * Tests the permission-based command functionality ensuring proper permission checks + * and message handling for various command scenarios. + */ class PermissionCommandTest extends CommandsSpigotTestBase { - PlayerMock player; + private final PlayerMock player; @BeforeEach @Override public void setup() { super.setup(); + // Clean up any existing permissions + if (player != null) { + player.getEffectivePermissions().clear(); + } commandManager.addCommand(new PermissionCommand()); player = server.addPlayer("ikfir"); }
68-71: Add validation and improve helper method visibility.The helper method should:
- Be marked as private
- Include parameter validation
- Add documentation
- void addPermission(Player target, String permission) { + /** + * Adds a permission to the specified player. + * + * @param target The player to add the permission to + * @param permission The permission string to add + * @throws IllegalArgumentException if target or permission is null + */ + private void addPermission(Player target, String permission) { + if (target == null) throw new IllegalArgumentException("Target cannot be null"); + if (permission == null) throw new IllegalArgumentException("Permission cannot be null"); + if (permission.isEmpty()) throw new IllegalArgumentException("Permission cannot be empty"); + PermissionAttachment attachment = target.addAttachment(plugin); attachment.setPermission(permission, true); }commands/spigot-platform/src/test/java/net/apartium/cocoabeans/commands/spigot/requirements/PermissionCommand.java (2)
13-17: Consider extracting the message string and adding documentation.While the implementation is functionally correct, consider these improvements:
- Extract the message string to a constant or resource bundle for better i18n support
- Add Javadoc explaining the interaction between
@Permissionand@SubCommandannotations+ /** + * Tests permission-based command execution. + * Requires 'my.test' permission to execute. + * + * @param sender the command sender + */ @Permission("my.test") @SubCommand("test") public void testWithPermission(CommandSender sender) { - sender.sendMessage("You got a permission"); + sender.sendMessage(Messages.PERMISSION_GRANTED); }
1-37: Architectural concern: Implementation code in test directory.This file appears to contain actual command implementation but is located in the test directory (
src/test/java). If this is intended to be production code, it should be moved tosrc/main/java. If it's a test class, it should contain test assertions and extend a test framework class.
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
📒 Files selected for processing (3)
commands/spigot-platform/src/test/java/net/apartium/cocoabeans/commands/spigot/requirements/PermissionCommand.java(1 hunks)commands/spigot-platform/src/test/java/net/apartium/cocoabeans/commands/spigot/requirements/PermissionCommandTest.java(1 hunks)commands/spigot-platform/src/test/java/net/apartium/cocoabeans/commands/spigot/requirements/factory/PermissionFactoryTest.java(1 hunks)
🔇 Additional comments (3)
commands/spigot-platform/src/test/java/net/apartium/cocoabeans/commands/spigot/requirements/factory/PermissionFactoryTest.java (1)
11-13: Verify test alignment with PermissionFactory implementation
Let's verify that this test aligns with the actual implementation of PermissionFactory.
✅ Verification successful
Test correctly verifies null handling in PermissionFactory
The test aligns perfectly with the implementation. The getRequirement method in PermissionFactory returns null when the obj parameter is not an instance of Permission, which includes the case when obj is null. The test correctly verifies this behavior by asserting that null is returned when both parameters are null.
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash
# Description: Analyze PermissionFactory implementation to verify test assumptions
# Find and examine PermissionFactory class implementation
echo "Searching for PermissionFactory implementation..."
rg -l "class PermissionFactory"
# Look for getRequirement method signature
echo "Analyzing getRequirement method signature..."
ast-grep --pattern 'class PermissionFactory {
$$$
getRequirement($$$) {
$$$
}
$$$
}'
Length of output: 682
Script:
#!/bin/bash
# Let's examine the actual implementation of PermissionFactory in the spigot-platform
echo "Content of PermissionFactory implementation:"
cat commands/spigot-platform/src/main/java/net/apartium/cocoabeans/commands/spigot/requirements/factory/PermissionFactory.java
# Also check the test file for context
echo -e "\nContent of PermissionFactoryTest:"
cat commands/spigot-platform/src/test/java/net/apartium/cocoabeans/commands/spigot/requirements/factory/PermissionFactoryTest.java
Length of output: 4941
commands/spigot-platform/src/test/java/net/apartium/cocoabeans/commands/spigot/requirements/PermissionCommandTest.java (1)
1-73: Verify test coverage against PermissionCommand implementation.
Let's ensure all paths in the PermissionCommand class are covered by these tests.
✅ Verification successful
Test coverage is complete and well-structured
The test coverage for PermissionCommand is comprehensive, covering all implemented paths:
testWithPermissionmethod is covered bytestWithPermissionandtestWithAndWithoutPermissionteststestWithoutPermissionmethod is covered bytestWithoutPermissionandtestWithAndWithoutPermissionteststestWithAndWithoutPermissionmethod is covered bywowTestWithAndWithoutPermissiontest- Permission handling and exception cases are verified through the combination of tests with and without required permissions
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash
# Description: Find the PermissionCommand implementation to verify coverage
# Find the PermissionCommand class
ast-grep --pattern 'class PermissionCommand {
$$$
}'
# Find all command methods that might need testing
ast-grep --pattern 'public $_ execute($$$) {
$$$
}'
Length of output: 2841
Script:
#!/bin/bash
# Let's find the actual PermissionCommand implementation
fd PermissionCommand.java --type f --exclude '*Test.java' --exec cat {}
Length of output: 1328
commands/spigot-platform/src/test/java/net/apartium/cocoabeans/commands/spigot/requirements/PermissionCommand.java (1)
1-11: LGTM! Clean class structure and proper imports.
The class structure follows good practices with appropriate annotations and interface implementation.
...java/net/apartium/cocoabeans/commands/spigot/requirements/factory/PermissionFactoryTest.java
Outdated
Show resolved
Hide resolved
...java/net/apartium/cocoabeans/commands/spigot/requirements/factory/PermissionFactoryTest.java
Outdated
Show resolved
Hide resolved
...rc/test/java/net/apartium/cocoabeans/commands/spigot/requirements/PermissionCommandTest.java
Show resolved
Hide resolved
...rm/src/test/java/net/apartium/cocoabeans/commands/spigot/requirements/PermissionCommand.java
Show resolved
Hide resolved
...rm/src/test/java/net/apartium/cocoabeans/commands/spigot/requirements/PermissionCommand.java
Show resolved
Hide resolved
There was a problem hiding this comment.
Actionable comments posted: 1
🧹 Outside diff range and nitpick comments (2)
Writerside/topics/compound-parser.md (2)
1-15: Fix documentation formatting and grammar issuesSeveral improvements needed in the documentation:
- Fix hyphenation: "error-prone" instead of "error prone"
- Fix grammar: "it's" instead of "its" in "the same way its possible"
- Fix list indentation in the Advantages section
- Fix duplicate content in advantages (the "Reduced Error Prone" point repeats the same text as "Easier to Use")
Apply these changes:
-Writing custom parsers for complicated arguments, such as locations, can be a tedious and error prone task. -For this reason we have added a way to define compound parsers in the same way its possible to define commands. +Writing custom parsers for complicated arguments, such as locations, can be a tedious and error-prone task. +For this reason, we have added a way to define compound parsers in the same way it's possible to define commands. ## Advantages - **Improved Readability** 📖: The compound parser provides a clear and descriptive name for each variant, making the command structure more readable and self-explanatory. -- **Easier to Use** 👍: The compound parser is easy to use and can be used to create complex parsers with multiple parameters and variants with different requirements. -- **Reduced Code** 📦: The compound parser is a reusable component that can be used in multiple commands to avoid code duplication and reduce the amount of code needed to write a command. -- **Increased Flexibility** 🎯: The compound parser allows you to create parsers that are more complex and can be used in multiple places in your command structure. -- **Reduced Error Prone** 🐛: The compound parser is easy to use and can be used to create complex parsers with multiple parameters and variants with different requirements. + **Improved Readability** 📖: The compound parser provides a clear and descriptive name for each variant, making the command structure more readable and self-explanatory. + **Easier to Use** 👍: The compound parser is easy to use and can be used to create complex parsers with multiple parameters and variants with different requirements. + **Reduced Code** 📦: The compound parser is a reusable component that can be used in multiple commands to avoid code duplication and reduce the amount of code needed to write a command. + **Increased Flexibility** 🎯: The compound parser allows you to create parsers that are more complex and can be used in multiple places in your command structure. + **Reduced Error Prone** 🐛: The compound parser helps minimize mistakes through its structured approach to parameter parsing and validation.Also applies to: 137-142
🧰 Tools
🪛 LanguageTool
[misspelling] ~13-~13: This word is normally spelled with a hyphen.
Context: ...such as locations, can be a tedious and error prone task. For this reason we have added a w...(EN_COMPOUNDS_ERROR_PRONE)
[typographical] ~13-~13: Use a comma after an introductory phrase.
Context: ... can be a tedious and error prone task. For this reason we have added a way to define compound ...(COMMA_INTRODUCTORY_WORDS_PHRASES)
[grammar] ~14-~14: Did you mean “it’s” (contraction of “it is/has”)?
Context: ...define compound parsers in the same way its possible to define commands. Each parse...(ITS_TO_IT_S)
95-96: Fix typo in section headingThe heading contains a typo: "warp" should be "wrap"
-#### How could we warp it? +#### How could we wrap it?🧰 Tools
🪛 Markdownlint (0.35.0)
95-95: Expected: h3; Actual: h4
Heading levels should only increment by one level at a time(MD001, heading-increment)
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
📒 Files selected for processing (3)
Writerside/topics/compound-parser.md(1 hunks)commands/spigot-platform/src/test/java/net/apartium/cocoabeans/commands/spigot/requirements/PermissionCommandTest.java(1 hunks)commands/spigot-platform/src/test/java/net/apartium/cocoabeans/commands/spigot/requirements/factory/PermissionFactoryTest.java(1 hunks)
🚧 Files skipped from review as they are similar to previous changes (2)
- commands/spigot-platform/src/test/java/net/apartium/cocoabeans/commands/spigot/requirements/factory/PermissionFactoryTest.java
- commands/spigot-platform/src/test/java/net/apartium/cocoabeans/commands/spigot/requirements/PermissionCommandTest.java
🧰 Additional context used
🪛 LanguageTool
Writerside/topics/compound-parser.md
[misspelling] ~13-~13: This word is normally spelled with a hyphen.
Context: ...such as locations, can be a tedious and error prone task. For this reason we have added a w...
(EN_COMPOUNDS_ERROR_PRONE)
[typographical] ~13-~13: Use a comma after an introductory phrase.
Context: ... can be a tedious and error prone task. For this reason we have added a way to define compound ...
(COMMA_INTRODUCTORY_WORDS_PHRASES)
[grammar] ~14-~14: Did you mean “it’s” (contraction of “it is/has”)?
Context: ...define compound parsers in the same way its possible to define commands. Each parse...
(ITS_TO_IT_S)
[uncategorized] ~97-~97: You might be missing the article “a” here.
Context: ...package-private */then we will create classLocationParser` that will be public ``...
(AI_EN_LECTOR_MISSING_DETERMINER_A)
[uncategorized] ~134-~134: A comma might be missing here.
Context: ...at will save you memory and computation time but the parsers you use must support mu...
(AI_EN_LECTOR_MISSING_PUNCTUATION_COMMA)
[uncategorized] ~135-~135: Use a comma before ‘but’ if it connects two independent clauses (unless they are closely connected and short).
Context: ...er every time. You could lock the parser but it's not recommended because it will sl...
(COMMA_COMPOUND_SENTENCE)
🪛 Markdownlint (0.35.0)
Writerside/topics/compound-parser.md
95-95: Expected: h3; Actual: h4
Heading levels should only increment by one level at a time
(MD001, heading-increment)
139-139: Expected: 1; Actual: 0
Inconsistent indentation for list items at the same level
(MD005, list-indent)
140-140: Expected: 1; Actual: 0
Inconsistent indentation for list items at the same level
(MD005, list-indent)
141-141: Expected: 1; Actual: 0
Inconsistent indentation for list items at the same level
(MD005, list-indent)
142-142: Expected: 1; Actual: 0
Inconsistent indentation for list items at the same level
(MD005, list-indent)
92-92: Expected: 0; Actual: 1
Unordered list indentation
(MD007, ul-indent)
93-93: Expected: 0; Actual: 1
Unordered list indentation
(MD007, ul-indent)
138-138: Expected: 0; Actual: 1
Unordered list indentation
(MD007, ul-indent)
🔇 Additional comments (1)
Writerside/topics/compound-parser.md (1)
49-51:
Add validation for location coordinates and rotation values
The location parsing methods should validate the input values to ensure they are within Minecraft's acceptable ranges.
This was previously raised in a past review. The implementation should:
- Validate coordinates against Minecraft's world limits
- Ensure yaw is normalized between 0 and 360 degrees
- Ensure pitch is constrained between -90 and 90 degrees
Also applies to: 54-57
...ain/java/net/apartium/cocoabeans/commands/spigot/requirements/factory/PermissionFactory.java
Outdated
Show resolved
Hide resolved
Closes #122
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Outside diff range and nitpick comments (1)
commands/spigot-platform/src/main/java/net/apartium/cocoabeans/commands/spigot/requirements/factory/PermissionFactory.java (1)
59-77: Consider extracting duplicated sender validationThe sender validation logic is duplicated between
meetsRequirementandmeetsInvertedRequirement. Consider extracting it to a private helper method:+ private RequirementResult validateSender(RequirementEvaluationContext context) { + Sender sender = context.sender(); + if (sender.getSender() == null || !(sender.getSender() instanceof CommandSender)) { + return RequirementResult.error(new UnmetPermissionResponse( + this, + context, + "You must be a command sender" + )); + } + return null; + } public RequirementResult meetsRequirement(RequirementEvaluationContext context) { if (invert) return meetsInvertedRequirement(context); - Sender sender = context.sender(); - if ((sender.getSender() == null || !(sender.getSender() instanceof CommandSender commandSender))) { - return RequirementResult.error(new UnmetPermissionResponse( - this, - context, - "You must be a command sender" - )); - } + RequirementResult validation = validateSender(context); + if (validation != null) return validation; + CommandSender commandSender = (CommandSender) context.sender().getSender(); // ... rest of the method } private RequirementResult meetsInvertedRequirement(RequirementEvaluationContext context) { - Sender sender = context.sender(); - if (sender.getSender() == null || !(sender.getSender() instanceof CommandSender commandSender)) - return RequirementResult.error(new UnmetPermissionResponse( - this, - context, - "You must be a command sender" - )); + RequirementResult validation = validateSender(context); + if (validation != null) return validation; + CommandSender commandSender = (CommandSender) context.sender().getSender(); // ... rest of the method }
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
📒 Files selected for processing (4)
CHANGELOG.md(2 hunks)Writerside/topics/Examples.md(1 hunks)commands/spigot-platform/src/main/java/net/apartium/cocoabeans/commands/spigot/requirements/factory/PermissionFactory.java(4 hunks)commands/src/test/java/net/apartium/cocoabeans/commands/parsers/compound/PositionParserTest.java(1 hunks)
🚧 Files skipped from review as they are similar to previous changes (3)
- Writerside/topics/Examples.md
- commands/src/test/java/net/apartium/cocoabeans/commands/parsers/compound/PositionParserTest.java
- CHANGELOG.md
🔇 Additional comments (4)
commands/spigot-platform/src/main/java/net/apartium/cocoabeans/commands/spigot/requirements/factory/PermissionFactory.java (4)
13-13: Remove unused CommandNode import
The CommandNode import is no longer used in the code as the method signature has been changed to use GenericNode. You can safely remove this unused import.
Line range hint 25-29: LGTM: Clean implementation of generic node handling
The change to use GenericNode makes the factory more flexible while maintaining type safety through pattern matching.
36-47: LGTM: Clean control flow separation
The extraction of inverted permission logic and clear condition grouping improves code readability.
62-67: Verify sender validation behavior
The current implementation fails with an error message when the sender is invalid. This seems correct as both normal and inverted permission checks require a valid CommandSender to evaluate permissions.
✅ Verification successful
The previous pattern might have been too specific. Let's try with a more flexible search to understand the sender validation patterns across the codebase and specifically in permission-related requirements.
Based on the search results, I can now provide a conclusive response about the sender validation behavior:
Sender validation is consistent with platform requirements
The sender validation in PermissionFactory is correct and consistent with other implementations in the codebase:
- The pattern of checking
sender.getSender() == null || !(sender.getSender() instanceof CommandSender)is used consistently across multiple requirement implementations (SenderType.java, InGameFactory.java, WhitelistRequirementFactory.java). - The error message "You must be a command sender" is appropriate as permissions can only be checked on Bukkit's CommandSender instances.
- The implementation correctly handles both normal and inverted permission checks by validating the sender first, as seen in the
meetsRequirementandmeetsInvertedRequirementmethods.
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash
# Search for similar sender validation patterns in other requirement implementations
ast-grep --pattern 'if ($sender.getSender() == null || !($sender.getSender() instanceof CommandSender)) {
$$$
}'
Length of output: 116
Script:
#!/bin/bash
# Let's search for similar sender validation patterns with a more flexible AST pattern
ast-grep --pattern 'if ($sender.getSender() == null || !($sender.getSender() instanceof $_)) {
$$$
}'
# Also search for other usages of getSender() method
rg "getSender\(\)" -A 3
# Let's also check for other permission-related requirement implementations
fd ".*Permission.*\.java$" --exec cat {}
Length of output: 24175
There was a problem hiding this comment.
Actionable comments posted: 8
🧹 Outside diff range and nitpick comments (6)
commands/src/test/java/net/apartium/cocoabeans/commands/RegisteredVariantTest.java (1)
10-10: Add class-level documentation.Consider adding a Javadoc comment to describe the purpose of this test class and what aspects of
RegisteredVariantit verifies.+/** + * Unit tests for {@link RegisteredVariant} class. + * Verifies the equality comparison logic between different instances + * of RegisteredVariant. + */ public class RegisteredVariantTest {commands/src/test/java/net/apartium/cocoabeans/commands/parsers/compound/AnotherEvilCompoundParser.java (2)
8-12: Improve class naming and documentationThe class name "AnotherEvilCompoundParser" doesn't reflect its testing purpose. Consider renaming it to something more descriptive like "TestStringCompoundParser".
14-17: Add documentation for parser variant methodThe
toOkmethod lacks documentation explaining its purpose in the test scenario.+/** + * Test parser variant that always returns "ok". + * Used to verify basic parser variant functionality. + * + * @return constant string "ok" + */ @ParserVariant("ok") public String toOk() { return "ok"; }commands/src/test/java/net/apartium/cocoabeans/commands/parsers/compound/EvilCompoundParser.java (1)
1-60: Add class-level documentationThe class lacks documentation explaining its role in testing the compound parser functionality.
+/** + * Test implementation of CompoundParser that handles Instant values. + * This class is used to verify the compound parser's behavior with temporal data + * and custom token handling. + */ public class EvilCompoundParser extends CompoundParser<Instant> {commands/src/test/java/net/apartium/cocoabeans/commands/parsers/compound/PersonCompoundParser.java (1)
12-18: Consider extracting the default keyword as a constant.The default keyword "person" should be extracted as a private static final constant for better maintainability and reusability.
public class PersonCompoundParser extends CompoundParser<PersonCompoundParser.Person> { + private static final String DEFAULT_KEYWORD = "person"; public PersonCompoundParser(String keyword, int priority) { super(keyword, Person.class, priority, new SimpleArgumentMapper(), new SimpleCommandLexer()); } public PersonCompoundParser(int priority) { - this("person", priority); + this(DEFAULT_KEYWORD, priority); }commands/src/test/java/net/apartium/cocoabeans/commands/parsers/compound/PersonCompoundParserTest.java (1)
39-41: Enhance the args helper method robustness.The current implementation could be improved to handle edge cases better.
static String[] args(String args) { - return args.split("\\s+"); + if (args == null || args.trim().isEmpty()) { + return new String[0]; + } + return args.trim().split("\\s+"); }
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
📒 Files selected for processing (9)
commands/src/main/java/net/apartium/cocoabeans/commands/requirements/argument/RangeArgumentRequirementFactory.java(3 hunks)commands/src/test/java/net/apartium/cocoabeans/commands/GeneralCommandTest.java(3 hunks)commands/src/test/java/net/apartium/cocoabeans/commands/RegisteredVariantTest.java(1 hunks)commands/src/test/java/net/apartium/cocoabeans/commands/ThereAnotherEvilCommand.java(1 hunks)commands/src/test/java/net/apartium/cocoabeans/commands/parsers/compound/AnotherEvilCompoundParser.java(1 hunks)commands/src/test/java/net/apartium/cocoabeans/commands/parsers/compound/EvilCompoundParser.java(1 hunks)commands/src/test/java/net/apartium/cocoabeans/commands/parsers/compound/EvilCompoundParserTest.java(1 hunks)commands/src/test/java/net/apartium/cocoabeans/commands/parsers/compound/PersonCompoundParser.java(1 hunks)commands/src/test/java/net/apartium/cocoabeans/commands/parsers/compound/PersonCompoundParserTest.java(1 hunks)
🚧 Files skipped from review as they are similar to previous changes (1)
- commands/src/test/java/net/apartium/cocoabeans/commands/GeneralCommandTest.java
🔇 Additional comments (9)
commands/src/test/java/net/apartium/cocoabeans/commands/ThereAnotherEvilCommand.java (1)
6-9: 🛠️ Refactor suggestion
Improve test documentation and parameter validation.
Several issues need to be addressed:
- The empty string in
@Param("")should include a parameter name for clarity - The method name
meowdoesn't follow command naming conventions - Missing Javadoc explaining the test scenario
- The exception message could be more descriptive
- @SubCommand("<int>")
- public void meow(Sender sender, @Param("") int num) {
- throw new UnsupportedOperationException("how did we get here");
- }
+ /**
+ * Tests handling of unreachable command paths.
+ * This command should never be executed as it represents an invalid state.
+ *
+ * @param sender the command sender
+ * @param value the test parameter
+ * @throws UnsupportedOperationException always, as this command path should be unreachable
+ */
+ @SubCommand("<int>")
+ public void unreachableCommand(Sender sender, @Param("value") int value) {
+ throw new UnsupportedOperationException("Unreachable command path - this execution path should never be reached");
+ }Let's verify if there are any similar test cases in the codebase:
commands/src/test/java/net/apartium/cocoabeans/commands/RegisteredVariantTest.java (2)
1-9: LGTM! Clean and well-organized imports.
The imports follow best practices with specific imports and appropriate static imports for assertions.
1-43: Consider adding integration tests.
While the unit tests for equality are important, consider adding integration tests to verify how RegisteredVariant works within the command system, especially around parameter parsing and command execution.
Let's check for existing integration tests:
commands/src/main/java/net/apartium/cocoabeans/commands/requirements/argument/RangeArgumentRequirementFactory.java (2)
14-14: Import GenericNode to Reflect Parameter Type Change
The addition of the import statement for GenericNode aligns with the updated method signature and ensures that the class compiles correctly.
24-24: 🛠️ Refactor suggestion
Verify Impact of Changing Parameter Type to GenericNode
Changing the parameter type from CommandNode to GenericNode in the getArgumentRequirement method increases flexibility but may affect subclasses and implementations that rely on the previous method signature. Ensure all implementations and usages are updated to prevent runtime issues.
Run the following script to find all usages and implementations of getArgumentRequirement:
commands/src/test/java/net/apartium/cocoabeans/commands/parsers/compound/EvilCompoundParser.java (1)
16-20: Review constructor accessibility
The class is public but the constructor is protected. This seems inconsistent with the class's intended use in tests.
commands/src/test/java/net/apartium/cocoabeans/commands/parsers/compound/PersonCompoundParser.java (2)
1-10: LGTM! Well-structured parser implementation.
The class is properly structured with appropriate annotations and inheritance. The use of @WithParser annotations clearly indicates the dependency on IntParser and StringParser.
25-30: LGTM! Good use of record for immutable data.
The Person record is appropriately used here for an immutable value object.
commands/src/test/java/net/apartium/cocoabeans/commands/parsers/compound/PersonCompoundParserTest.java (1)
1-13: LGTM! Well-structured test setup.
The test class follows good JUnit practices with proper annotations and setup.
commands/src/test/java/net/apartium/cocoabeans/commands/ThereAnotherEvilCommand.java
Show resolved
Hide resolved
commands/src/test/java/net/apartium/cocoabeans/commands/RegisteredVariantTest.java
Show resolved
Hide resolved
.../net/apartium/cocoabeans/commands/requirements/argument/RangeArgumentRequirementFactory.java
Outdated
Show resolved
Hide resolved
.../src/test/java/net/apartium/cocoabeans/commands/parsers/compound/EvilCompoundParserTest.java
Show resolved
Hide resolved
...ands/src/test/java/net/apartium/cocoabeans/commands/parsers/compound/EvilCompoundParser.java
Show resolved
Hide resolved
...ands/src/test/java/net/apartium/cocoabeans/commands/parsers/compound/EvilCompoundParser.java
Show resolved
Hide resolved
...ds/src/test/java/net/apartium/cocoabeans/commands/parsers/compound/PersonCompoundParser.java
Show resolved
Hide resolved
...rc/test/java/net/apartium/cocoabeans/commands/parsers/compound/PersonCompoundParserTest.java
Show resolved
Hide resolved
Closes #122
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Outside diff range and nitpick comments (1)
commands/src/main/java/net/apartium/cocoabeans/commands/requirements/argument/RangeArgumentRequirementFactory.java (1)
14-14: LGTM! Good architectural improvementThe change from
CommandNodetoGenericNodeimproves the flexibility and reusability of the factory. This aligns well with the PR's objective of creating more composable parsers.Also applies to: 24-24
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
📒 Files selected for processing (1)
commands/src/main/java/net/apartium/cocoabeans/commands/requirements/argument/RangeArgumentRequirementFactory.java(3 hunks)
🔇 Additional comments (2)
commands/src/main/java/net/apartium/cocoabeans/commands/requirements/argument/RangeArgumentRequirementFactory.java (2)
42-46: LGTM! Improved numeric type handling implemented
The suggested improvements from the previous review have been implemented correctly. The code now:
- Handles all numeric types uniformly using the Number interface
- Uses doubleValue() for safe conversion without precision loss
- Is more concise and maintainable
Line range hint 46-54: Consider floating-point precision in step validation
The modulo operation with floating-point numbers might lead to precision errors. For example, (1.1 - 1.0) % 0.1 might not exactly equal 0 due to floating-point arithmetic.
Consider adding a small epsilon value for comparison:
- return (num - from) % step == 0;
+ double epsilon = 1e-10;
+ double remainder = (num - from) % step;
+ return remainder < epsilon || (step - remainder) < epsilon;Closes #122
|
❓ Type of change
📚 Description
Compound parser is a simple way to create custom parsers that are composed of other parsers
For example if we want to create a simple location parser it will be the following:
🧪 How Has This Been Tested?
Unit testing
📝 Checklist
Summary by CodeRabbit
Release Notes
New Features
CatCommandandMeowparser to manage cat-related commands.PositionParserfor parsing position-related commands.GenericNodeinterface.PermissionCommandfor managing permissions in commands.ParserVariantandParserVariantsannotations for flexible command parsing.AnotherEvilCompoundParserandEvilCompoundParserfor specialized command handling.PermissionFactoryfor improved permission handling.Documentation
PositionParser.Bug Fixes
Tests
PositionParserto validate parsing and tab completion.PermissionCommandfunctionality.PermissionFactoryto ensure null handling.RegisteredVariantequality logic validation.EvilCompoundParserandAnotherEvilCompoundParserinstantiation.