Skip to content

Remove dead config branches, correct the wiki link, and cover the unquoted-argument path - #19

Merged
dmccoystephenson merged 4 commits into
mainfrom
feature/command-and-config-cleanup
Aug 12, 2026
Merged

Remove dead config branches, correct the wiki link, and cover the unquoted-argument path#19
dmccoystephenson merged 4 commits into
mainfrom
feature/command-and-config-cleanup

Conversation

@dmccoystephenson

Copy link
Copy Markdown
Member

Summary

Three small, independent cleanups are batched here. All of them are confined to the command layer and ConfigService; no command syntax, permission node, or documented config key is changed.

  • Dead template branches removed. ConfigService.setConfigOption carried branches for config options named A and C, parsing them as an integer and a double. Neither is a real option — saveMissingConfigDefaultsIfNotPresent writes only version and debugMode, and CONFIG.md documents exactly those two — and both branches sat unreachable behind the surrounding isSet(option) guard. A new ConfigServiceTest characterizes the surviving branches, including that any option other than version and debugMode is now stored verbatim as a String.
  • Wiki link corrected. The bare /pl output advertised https://github.com/dmccoystephenson/PlayerLore/wiki, under the plugin's former owner. It now points at https://github.com/Dans-Plugins/PlayerLore/wiki, matching every other link the project publishes, and DefaultCommandTest asserts the URL so the link cannot drift again unnoticed.
  • Unquoted-argument path covered. AddCommand and EditCommand both reject arguments containing no double-quoted segment, but no test exercised that branch. Characterization tests were added to each; no production behaviour was changed for this item.

Closes #13
Closes #14
Closes #16

Test plan

  • mvn test — 26 tests, 0 failures (19 before this branch)
  • Regression evidence, empirical: with DefaultCommand.java and ConfigService.java stashed, execute_advertisesTheWikiUnderTheCurrentRepositoryOwner and setConfigOption_storesAnyOtherOptionAsString both FAIL; with them restored, all 26 PASS
  • Documentation sources of truth re-checked against the implementation — HelpCommand, COMMANDS.md, USER_GUIDE.md, and CONFIG.md all still describe the code accurately and needed no edit; CHANGELOG.md gained Removed and Fixed entries under [Unreleased]

Notes and deferrals

  • On DefaultCommand advertises a wiki URL under the former dmccoystephenson owner #14, the alternative the issue raised — replacing the wiki link with a pointer to the in-repo USER_GUIDE.md if the wiki turns out to be unpopulated — was not taken, because network access is unavailable from this environment and the wiki's contents could therefore not be confirmed. The namespace correction is right either way; swapping the link for in-repo documentation remains available as a follow-up should a maintainer confirm the wiki is empty.
  • pl.default permission is declared in DefaultCommand but is absent from plugin.yml and USER_GUIDE.md #12 was deliberately skipped this cycle. That issue asks whether the unenforced pl.default node should be dropped from DefaultCommand or instead declared in plugin.yml, documented, and routed through the command service. The issue itself states that a maintainer decision on direction is needed, and either resolution would touch plugin.yml, a path this loop does not modify autonomously.
  • No other open issues existed at triage time.

This PR description was drafted during a Gardener session (https://github.com/Stephenson-Software/gardener).


drafted by Claude on behalf of Daniel Stephenson

dmccoystephenson and others added 4 commits August 12, 2026 02:01
Neither option exists: saveMissingConfigDefaultsIfNotPresent writes only
version and debugMode, and CONFIG.md documents exactly those two. Both
branches were also unreachable behind the surrounding isSet(option) guard.

Adds ConfigServiceTest to characterize the surviving branches of
setConfigOption, including that any option other than version and
debugMode is now stored verbatim as a String.

Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
The bare /pl output still advertised the wiki under the plugin's former
dmccoystephenson owner, which is inconsistent with every other link the
project publishes. DefaultCommandTest now asserts the advertised URL so
the link cannot drift again unnoticed.

Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
Both commands reject arguments containing no double-quoted segment, but
no test exercised that branch. ArgumentParser returns an empty list for
unquoted input rather than throwing, so the branch is reachable with
arguments such as /pl add hello.

Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
Verifying never().set("debugMode", true) would have passed had the code
written a different value to the same key. The two rejection tests now
assert that no key is written at all and that the config is not saved.

Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
@dmccoystephenson

Copy link
Copy Markdown
Member Author

Self-review rubric

Scored adversarially against the diff and command output, not from judgement. Two items were scored FAIL on the first pass and have since been fixed in f0e6b12.

  • Scope: PASS — seven files, all necessary. ConfigService.java + ConfigServiceTest.java serve ConfigService.setConfigOption contains dead template branches for nonexistent config options "A" and "C" #13, DefaultCommand.java + DefaultCommandTest.java serve DefaultCommand advertises a wiki URL under the former dmccoystephenson owner #14, the AddCommandTest/EditCommandTest additions serve Command tests do not cover the non-empty-but-unquoted argument path #16, and CHANGELOG.md records the two user-visible items. No unrelated formatting, rename, or comment churn appears in gh pr diff 19. Non-test net change is −4 LOC across two files, far under the scope ceiling.
  • Tests-new: PASS — no new public method or function is introduced by this PR, so the item applies vacuously on the production side. On the test side, all four surviving paths through setConfigOption are now exercised: option absent, version refused, debugMode parsed as a boolean, and everything else stored verbatim.
  • Tests-fix: PASS — confirmed empirically rather than by reasoning. With DefaultCommand.java and ConfigService.java stashed, mvn test reported Tests run: 26, Failures: 2, the two failures being DefaultCommandTest.execute_advertisesTheWikiUnderTheCurrentRepositoryOwner and ConfigServiceTest.setConfigOption_storesAnyOtherOptionAsString. With both files restored, Tests run: 26, Failures: 0. The A-as-a-String assertion was chosen precisely so the removal is observable; a neutrally-named option would have passed either way and given a false negative.
  • Sibling structure: PASSConfigServiceTest is the first test under services/, so commands/ was used as the reference: same public class XTest shape, same static-import block, same @BeforeEach setUp() building mocks by hand, same mock()/when()/verify() idiom, no MockitoExtension. It mirrors the main source package layout as the existing tests do.
  • Sibling renames: PASS — nothing is renamed by this PR.
  • Docs: PASS — every row of the sources-of-truth table was re-checked against the implementation. HelpCommand (no command syntax changed), COMMANDS.md (same four rows, same permissions), and USER_GUIDE.md (permissions table still matches plugin.yml, which is untouched) needed no edit. CONFIG.md still documents version and debugMode, which remain exactly what saveMissingConfigDefaultsIfNotPresent writes — removing the A/C branches changed no documented key. CHANGELOG.md gained Removed and Fixed entries under [Unreleased], in Keep a Changelog section order.
  • Issue resolution: PASS, with one deferral recordedDefaultCommand advertises a wiki URL under the former dmccoystephenson owner #14's URL and Command tests do not cover the non-empty-but-unquoted argument path #16's two uncovered branches are fully addressed. ConfigService.setConfigOption contains dead template branches for nonexistent config options "A" and "C" #13's named surface area, the A and C branches, is removed. That issue's separate "Related observation" — that setConfigOption and sendConfigList are unreachable from any registered command — is not resolved here, as the issue itself asks for it to be handled as separate work; it has been carried into ConfigService.setConfigOption and sendConfigList are unreachable from any registered command #20 so it is not lost when ConfigService.setConfigOption contains dead template branches for nonexistent config options "A" and "C" #13 closes.
  • Command trio parity: PASS — no production change was made to AddCommand, EditCommand, or RemoveCommand, so no validation or permission logic needed propagating. On the test side, RemoveCommand was checked directly and contains no ArgumentParser usage, which confirms Command tests do not cover the non-empty-but-unquoted argument path #16's claim that it needs no equivalent unquoted-argument test.
  • CI: PASSbuild passes on head f0e6b12 (run 31576633314). mvn clean package runs the suite, so the anchor's scope covers every file this PR touches; there is no untested-by-CI platform-specific path here.

Findings fixed during review

  • src/test/java/dansplugins/playerlore/services/ConfigServiceTest.java:41 — the negative assertion read verify(config, never()).set("debugMode", true), which would have passed had the code written a different value to that key, or written to a different key entirely. Both rejection tests now assert never()).set(anyString(), any()) instead.
  • src/test/java/dansplugins/playerlore/services/ConfigServiceTest.java:53 — the same weakness on the version path, compounded by no assertion that the config is left unsaved. verify(playerLore, never()).saveConfig() was added to both rejection tests.

Observations carried in this body rather than inline

  • The wiki's contents could not be confirmed, because network access is unavailable from this environment. The namespace correction is correct regardless of what the wiki holds, but if a maintainer confirms the wiki is unpopulated, replacing the link with a pointer to the in-repo USER_GUIDE.md would serve players better and remains an easy follow-up.
  • ConfigServiceTest.setConfigOption_storesAnyOtherOptionAsString deliberately keeps the name A alive in a test after removing it from production code. This reads oddly out of context and is explained by a comment at the assertion. Should ConfigService.setConfigOption and sendConfigList are unreachable from any registered command #20 be resolved by deleting setConfigOption outright, this test goes with it.

This review was performed and posted during a Gardener session (https://github.com/Stephenson-Software/gardener).


drafted by Claude on behalf of Daniel Stephenson

@dmccoystephenson
dmccoystephenson merged commit 16448e9 into main Aug 12, 2026
1 check passed
@dmccoystephenson
dmccoystephenson deleted the feature/command-and-config-cleanup branch August 12, 2026 08:05
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

1 participant