Skip to content

Allow /at top to accept an optional number of players - #83

Merged
dmccoystephenson merged 1 commit into
mainfrom
feature/top-command-count-argument
Aug 7, 2026
Merged

Allow /at top to accept an optional number of players#83
dmccoystephenson merged 1 commit into
mainfrom
feature/top-command-count-argument

Conversation

@dmccoystephenson

Copy link
Copy Markdown
Member

Summary

  • /at top now accepts an optional number argument (/at top 25); the default remains 10 and the maximum accepted value is 100, so a single command cannot flood chat with an unbounded leaderboard.
  • ActivityRecordService.getTopRecords(int count) was added, and getTopTenRecords() now delegates to it. The existing REST leaderboard endpoint keeps calling getTopTenRecords(), so its behavior and documented response shape are unchanged.
  • Argument validation follows the style already established by /at average: a non-numeric argument, zero or a negative number, and a number above 100 are each rejected with a red message and no leaderboard output.
  • The in-game help output, COMMANDS.md, USER_GUIDE.md and CHANGELOG.md were updated to describe the new argument.

Closes #38

Test plan

  • mvn -B test — 47 tests run, 0 failures (33 pre-existing plus 14 new).
  • TopCommandTest (new, JUnit 5): default of 10 with no argument and via the sender-only overload, the requested count for a valid argument, the boundary value 100, and rejection of abc, 0, -5 and 101 with the service never consulted.
  • ActivityRecordServiceTest (new, JUnit 5): ordering by hours descending, the requested count honored, fewer records than requested, zero requested, a negative count propagating IllegalArgumentException from the algorithm, and getTopTenRecords() still capping at 10.
  • In-game smoke test on a Spigot server — not run in this environment; the plugin's Bukkit command path has no automated coverage.

Notes on test scope

New tests were written against JUnit 5 deliberately: the JUnit 4 classes in this repository are not currently executed by Surefire, which is filed separately as #80. A timing-flaky assertion observed while investigating that is filed as #81, and a CONTRIBUTING.md reference to a non-existent develop branch is filed as #82. None of the three is addressed here, to keep this change scoped to #38 and out of pom.xml.

Deferred this cycle

The remaining open issues were not picked up, for the reasons below:

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

- Add ActivityRecordService.getTopRecords(int count); getTopTenRecords() now delegates to it
- Parse and validate an optional count argument in TopCommand (1-100, default 10)
- Add /at top (number) to the in-game help output
- Document the argument in COMMANDS.md, USER_GUIDE.md and CHANGELOG.md
- Add JUnit 5 tests for TopCommand argument handling and ActivityRecordService.getTopRecords

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

Copy link
Copy Markdown
Member Author

Self-review

Rubric scored against the diff and command output, not from memory.

  • Scope: PASS (with one note) — all six modified files and two new test files serve Allow players to specify how many players they want to see by typing /at top (number). #38. One note: four trailing-whitespace-only lines were normalized inside ActivityRecordService.getTopRecords, the block that was being rewritten anyway; no unrelated file was reformatted.
  • Tests-new: PASS — the one new public method, ActivityRecordService.getTopRecords(int), is exercised by six cases in ActivityRecordServiceTest; the new private TopCommand.displayTopRecords path is exercised through the public execute overloads by eight cases in TopCommandTest.
  • Tests-fix: not applicableAllow players to specify how many players they want to see by typing /at top (number). #38 is a feature request, not a bug fix, so the stash-and-run FAIL→PASS check has nothing to revert. No regression claim is being made.
  • Sibling structure: PASSTopCommandTest and ActivityRecordServiceTest follow the JUnit 5 + @DisplayName shape of TopRecordsAlgorithmTest; the argument validation, the DEFAULT_COUNT constant placement, and the red rejection messages mirror AverageCommand; the two /at top help lines mirror the existing /at info and /at info (playerName) pair.
  • Sibling renames: PASS — the loop counter in displayTopRecords was renamed countrank because the new method parameter took that name; both the declaration and both use sites were renamed in the same commit (a shadowed name would not have compiled otherwise).
  • Docs: PASSCOMMANDS.md and USER_GUIDE.md describe the argument, its default of 10 and its cap of 100; CHANGELOG.md has an Unreleased entry; README.md links to those guides and needed no change; REST_API.md and openapi.yaml needed no change, since RestApiService still calls getTopTenRecords() and the /leaderboard response shape is untouched.
  • Issue resolution: PASSAllow players to specify how many players they want to see by typing /at top (number). #38 asks for /at top (number); TopCommand.execute(CommandSender, String[]) now parses that argument and passes it through to the service. Nothing in Allow players to specify how many players they want to see by typing /at top (number). #38 is left partially done.
  • CI: PASS, with a stated limitationbuild and test are both green on 598ba21. The build job's mvn clean verify ran all 47 tests (Tests run: 47, Failures: 0, Errors: 0, Skipped: 0), including the 14 new ones. Two caveats are recorded rather than glossed over: (a) Simple CI only compiles and exercises TopRecordsAlgorithm.java standalone, which this PR does not touch, so that green check verifies nothing about this change; (b) the ci-test profile sets maven.test.failure.ignore and activates on CI, so the build check would have stayed green even on a test failure — this is filed as The ci-test profile makes test failures non-fatal in CI #84. The binding verification for this PR is therefore the local mvn -B test run (47 tests, 0 failures, profile inactive), not the badge.
  • REST API doc parity: not applicable — no HTTP endpoint was added or changed.
  • Algorithm CI scope honored: not applicablealgorithms/TopRecordsAlgorithm.java was not modified. getTopRecords(List, int) was already public and already covered by the inline SimpleTest assertions in simple-ci.yml; this PR only adds a second caller of it.

Judgment calls left for a reviewer

  • src/main/java/dansplugins/activitytracker/commands/TopCommand.java:110 — a count above 100 is rejected with a message rather than clamped to 100. Rejection was chosen for predictability and to match how /at average rejects bad input, but clamping would be friendlier; either is defensible.
  • src/main/java/dansplugins/activitytracker/commands/TopCommand.java:96 — arguments after the first are ignored, so /at top 5 extra behaves as /at top 5. This matches the existing tolerance of /at average and was left alone deliberately.
  • No in-game smoke test was possible in this environment; the Bukkit command path has no automated coverage beyond the mocked CommandSender used here.

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

@dmccoystephenson
dmccoystephenson merged commit d35d778 into main Aug 7, 2026
3 checks passed
@dmccoystephenson
dmccoystephenson deleted the feature/top-command-count-argument branch August 7, 2026 03:07
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

Allow players to specify how many players they want to see by typing /at top (number).

1 participant