Send usage message from /pl add instead of throwing on empty args - #15
Merged
Merged
Conversation
AddCommand passed args straight into ArgumentParser.getArgumentsInsideDoubleQuotes, which throws IllegalArgumentException when the array is empty. EditCommand and RemoveCommand already guard against an empty args array before parsing; AddCommand now does the same and sends its usage message. The characterization test that asserted the thrown exception is replaced with a regression test matching the sibling execute_rejectsMissingArgs tests. Closes #10 Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
Member
Author
Self-review rubricScored against the diff and against command output, on head
Findings not scored as rubric failuresAn all-PASS rubric warrants scrutiny, so the following were looked for specifically and are recorded as observations rather than blockers.
Verdict: every rubric item passes with evidence, and the three observations above are maintainability and coverage notes rather than defects in this change. This review was drafted during a Gardener session (https://github.com/Stephenson-Software/gardener). |
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Summary
args.length == 0guard is added toAddCommand.execute(CommandSender, String[]). Previously the empty array was passed straight intoArgumentParser.getArgumentsInsideDoubleQuotes(args), which throwsIllegalArgumentException: Arguments not valid.; the usage messageUsage: /pl add "line of lore"is now sent andfalsereturned instead.EditCommandandRemoveCommand, restoring parity across the add/edit/remove trio for empty-argument handling.execute_missingArgs_throwsInsteadOfSendingUsageMessage, which asserted the buggy behaviour, is replaced byexecute_rejectsMissingArgs, named and shaped to match the equivalent tests inEditCommandTestandRemoveCommandTest. The exact usage string is asserted so that drift is caught.[Unreleased]/Fixedentry is added toCHANGELOG.md.Test plan
mvn -o clean test— 19 tests, 0 failures, 0 errors.AddCommand.javachange stashed,execute_rejectsMissingArgserrors withIllegalArgumentException: Arguments not valid.; with the change restored, it passes.HelpCommand.java,COMMANDS.md,USER_GUIDE.md, andCONFIG.mddescribe no syntax, permission, or config surface that this change alters, so onlyCHANGELOG.mdrequired updating./pl addwith no arguments on a live Spigot server. The CI workflow builds and runs the unit suite only; no server-backed test exists in this repository.Notes on unquoted arguments
ArgumentParser.getArgumentsInsideDoubleQuoteswas probed directly to confirm the guard is placed correctly. Inputs{"foo"},{"foo", "bar"}, and{"\"unterminated"}all return an empty list rather than throwing, so the pre-existingdoubleQuoteArgs.size() == 0branch already handles those cases. Only the genuinely empty array threw, which is exactly what this guard now intercepts.Issues deferred this cycle
Three findings were filed during triage and deliberately not implemented here, so that this PR stays scoped to a single bug fix:
pl.defaultis declared inDefaultCommandbut absent fromplugin.ymlandUSER_GUIDE.md. Deferred because a maintainer decision is required on whether the node should be removed or declared and enforced, and because a fix would touchplugin.yml.ConfigService.setConfigOptioncontains dead template branches for nonexistent options"A"and"C". Deferred because the adjacent question of whethersetConfigOption/sendConfigListshould exist at all is a product decision.DefaultCommandadvertises a wiki URL under the formerdmccoystephensonowner. Deferred pending confirmation that a wiki is populated, since linking to in-repo documentation may be preferable.Closes #10
This PR description was drafted during a Gardener session (https://github.com/Stephenson-Software/gardener).