Skip to content

Converge export sorting onto the key-based ProjectSorter.sortBy API - #45

Open
dmccoystephenson wants to merge 1 commit into
mainfrom
feat/converge-export-sorting
Open

Converge export sorting onto the key-based ProjectSorter.sortBy API#45
dmccoystephenson wants to merge 1 commit into
mainfrom
feat/converge-export-sorting

Conversation

@dmccoystephenson

Copy link
Copy Markdown
Member

Summary

  • The boolean-flag ProjectSorter.sortByScore(List, boolean) path has been removed, so comparator selection is no longer duplicated between it and the key-based sortBy(List, String) API added for the list command. sortByIce and sortByRice have been kept as thin delegating wrappers onto sortBy, which preserves their existing test coverage while leaving a single comparator-selection site.
  • Sort-key vocabulary has been given a single owner: ProjectSorter.DEFAULT_SORT_KEY, ProjectSorter.describeSortKey (e.g. "ICE score", "name") and ProjectSorter.describeSortDirection ("A to Z" / "highest to lowest") have been added and are consumed by both the export command and the markdown header.
  • ProjectMarkdownWriter.writeMarkdown and MarkdownFormatter.formatHeader now take a sort key rather than a boolean. As a side effect, every sort key is now available to export --sort, not only ice and rice.
  • Sorting is now performed before the output file is opened, so an unsupported sort key is rejected before projects.md is truncated. Previously an invalid key could only have been surfaced after the header had already been written.
  • export --sort is validated with ProjectSorter.isSupportedSortKey, reusing the error-message shape the list command uses (Invalid sort option. Use one of: name, impact, confidence, ease, reach, effort, ice, rice.).
  • The sortByScore test coverage has been migrated onto sortBy rather than deleted, and coverage has been added for the new description helpers, for a name-sorted export end to end, and for the unsupported-key-before-write behavior.
  • The README Features list, command list and Export Examples block have been updated to describe the wider set of export --sort values.

Behavior notes

  • export and export --sort rice are unchanged for users; the emitted header text for those keys is byte-identical to before.
  • export still always sorts (defaulting to ICE), which differs from list, where omitting --sort keeps creation order. This difference has been documented in the README rather than changed, since altering it would change existing export behavior beyond the scope of this issue.

Test plan

  • ./gradlew compileJava — BUILD SUCCESSFUL
  • ./gradlew test — BUILD SUCCESSFUL, 87 tests, 0 failures
  • End-to-end markdown export verified for ice, rice and name keys in MarkdownExportIntegrationTest

Scope

Eleven files were modified, one over the ten-file soft ceiling; five of the eleven are test files and the non-test net change is well under the LOC ceiling. The remaining open issues (#37, #36, #33, #32, #27, #21, #20, #8) were deferred because this cycle was scoped to a single coherent refactor of the export sorting path; none of them touch it.

Closes #43

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


drafted by Claude on behalf of Daniel Stephenson

…rtBy API

Replaces the boolean-flag sortByScore path with the key-based sortBy API so
comparator selection lives in one place, and widens export --sort to accept
every sort key the list command supports.

- Remove ProjectSorter.sortByScore(List, boolean); sortByIce/sortByRice now
  delegate to sortBy
- Add ProjectSorter.DEFAULT_SORT_KEY, describeSortKey and describeSortDirection
  so sort-key vocabulary has a single owner
- Change ProjectMarkdownWriter.writeMarkdown and MarkdownFormatter.formatHeader
  to take a sort key, and sort before opening the file so an unsupported key is
  rejected before the file is touched
- Validate export --sort with ProjectSorter.isSupportedSortKey, matching the
  error message shape the list command uses
- Migrate the sortByScore test coverage onto sortBy and document the wider
  export sort keys in the README

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

Copy link
Copy Markdown

Test Results

87 tests  +9   87 ✅ +9   1s ⏱️ ±0s
14 suites ±0    0 💤 ±0 
14 files   ±0    0 ❌ ±0 

Results for commit 765a653. ± Comparison against base commit 9dad1cf.

This pull request removes 9 and adds 18 tests. Note that renamed tests count towards both.
com.preponderous.parpt.export.ProjectSorterTest ‑ sortByIce_ShouldDelegateToSortByScoreWithFalse()
com.preponderous.parpt.export.ProjectSorterTest ‑ sortByRice_ShouldDelegateToSortByScoreWithTrue()
com.preponderous.parpt.export.ProjectSorterTest ‑ sortByScore_ShouldNotModifyOriginalList()
com.preponderous.parpt.export.ProjectSorterTest ‑ sortByScore_WithEmptyList_ShouldReturnEmptyList()
com.preponderous.parpt.export.ProjectSorterTest ‑ sortByScore_WithEqualScores_ShouldMaintainStableOrder()
com.preponderous.parpt.export.ProjectSorterTest ‑ sortByScore_WithICESorting_ShouldSortByICEScore()
com.preponderous.parpt.export.ProjectSorterTest ‑ sortByScore_WithNullProjects_ShouldThrowException()
com.preponderous.parpt.export.ProjectSorterTest ‑ sortByScore_WithRICESorting_ShouldSortByRICEScore()
com.preponderous.parpt.export.ProjectSorterTest ‑ sortByScore_WithSingleProject_ShouldReturnSingleProjectList()
com.preponderous.parpt.command.ExportProjectsCommandTest ‑ execute_WithNonScoreSortKey_ShouldExportWithThatKey()
com.preponderous.parpt.export.MarkdownFormatterTest ‑ formatHeader_WithMixedCaseSortKey_ShouldFormatAsIfLowerCase()
com.preponderous.parpt.export.MarkdownFormatterTest ‑ formatHeader_WithNameSort_ShouldDescribeAlphabeticalOrder()
com.preponderous.parpt.export.MarkdownFormatterTest ‑ formatHeader_WithScoringFieldSort_ShouldDescribeDescendingOrder()
com.preponderous.parpt.export.MarkdownFormatterTest ‑ formatHeader_WithUnsupportedSortKey_ShouldThrowException()
com.preponderous.parpt.export.ProjectSorterTest ‑ defaultSortKey_ShouldBeASupportedKey()
com.preponderous.parpt.export.ProjectSorterTest ‑ describeSortDirection_ShouldDescribeNameAlphabeticallyAndEveryOtherKeyDescending()
com.preponderous.parpt.export.ProjectSorterTest ‑ describeSortDirection_WithUnsupportedKey_ShouldThrowException()
com.preponderous.parpt.export.ProjectSorterTest ‑ describeSortKey_ShouldDescribeEverySupportedKey()
com.preponderous.parpt.export.ProjectSorterTest ‑ describeSortKey_WithMixedCaseKey_ShouldDescribeAsIfLowerCase()
…

@dmccoystephenson

Copy link
Copy Markdown
Member Author

Self-review rubric (anchored on ./gradlew test and on the CI test job, both green on this head):

  • Scope: PASS — all eleven modified files trace to Converge export sorting onto the key-based ProjectSorter.sortBy API #43: five main-source files on the export sorting path, five mirroring test files, and the README section describing export --sort. No unrelated formatting or renames appear in gh pr diff 45.
  • Tests-new: PASSdescribeSortKey, describeSortDirection and DEFAULT_SORT_KEY are each exercised by new cases in ProjectSorterTest; the widened export --sort is covered by execute_WithNonScoreSortKey_ShouldExportWithThatKey; the new header shapes are covered by formatHeader_WithNameSort_ShouldDescribeAlphabeticalOrder and formatHeader_WithScoringFieldSort_ShouldDescribeDescendingOrder.
  • Tests-fix: PASS — verified empirically rather than by reasoning. With the sort-before-open reordering reverted in ProjectMarkdownWriterImpl (sorting moved back inside the try block, after the header write), writeMarkdown_WithUnsupportedSortKey_ShouldThrowBeforeWritingTheFile FAILED and writeMarkdown_WithEmptyList_ShouldWriteNoProjectsMessage FAILED on an unnecessary stub; with the change restored, all 87 tests PASS.
  • Sibling structure: PASS — no new files were created, so no directory conventions were introduced.
  • Sibling renames: PASS — the parallel pair sortByIce/sortByRice was re-pointed at sortBy in the same commit, and their tests were renamed together (sortByIce_ShouldDelegateToSortByWithIceKey, sortByRice_ShouldDelegateToSortByWithRiceKey). The sortByRicesortKey parameter rename was applied across the interface, the implementation, the formatter and all five test files.
  • Docs: PASSREADME.md Features, command list and Export Examples were updated; CONTRIBUTING.md was re-read and its documented commands (./gradlew test, ./gradlew build) are unaffected by this change.
  • Issue resolution: PARTIAL, by design — every bullet of Converge export sorting onto the key-based ProjectSorter.sortBy API #43 is addressed. The one open decision it posed ("kept as thin delegating wrappers or removed") was resolved by removing sortByScore(List, boolean) and keeping sortByIce/sortByRice; see the finding below.
  • Manual validation: PASS./gradlew test → BUILD SUCCESSFUL, 87 tests, 0 failures; the CI test job is green on this head.
  • Shell command tests: PASSExportProjectsCommandTest covers the changed @ShellMethod, including the widened key set, the new error message and case-insensitive keys.
  • Round-trip coverage: not applicable — no field was added to Project or any other Lombok domain class.
  • I/O behind an interface: PASS — no new filesystem or console access was introduced; ProjectMarkdownWriterImpl still writes through the existing ProjectMarkdownWriter seam.

Findings that call for a judgment rather than a mechanical fix:

  • src/main/java/com/preponderous/parpt/export/ProjectSorter.java:88sortByIce and sortByRice now have no production callers; ProjectMarkdownWriterImpl reaches sortBy directly and ListProjectsCommand always did. They were retained rather than deleted because Converge export sorting onto the key-based ProjectSorter.sortBy API #43 sanctions either choice and because deleting them would have discarded their existing test coverage, but they are effectively a test-only convenience API at this point and a reviewer may prefer them gone.
  • src/main/java/com/preponderous/parpt/command/ExportProjectsCommand.java:26 — the @ShellOption help text spells the supported keys out as a literal, duplicating the identical literal in ListProjectsCommand and the authoritative list in ProjectSorter.SUPPORTED_SORT_KEYS. Annotation values must be compile-time constants, so the list cannot be generated from getSupportedSortKeys() in place; this drift risk is now doubled rather than introduced, and a follow-up issue has been filed for it.
  • src/main/java/com/preponderous/parpt/repo/ProjectMarkdownWriter.java:4 — the repo interface now imports ProjectSorter from the export package, for DEFAULT_SORT_KEY and the Javadoc link. ProjectMarkdownWriterImpl already depended on ProjectSorter, so this follows the existing arrangement rather than establishing a new one, but it does move an export reference up into the interface.
  • The raw user-supplied key is passed through to writeMarkdown without normalization (so a mock sees "IcE", not "ice"), matching how ListProjectsCommand passes its key straight to sortBy. Normalization happens inside ProjectSorter, so the rendered header and the success message are canonical either way; this is asserted by execute_WithCaseInsensitiveSortOptions_ShouldWork.

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


drafted by Claude on behalf of Daniel Stephenson

@dmccoystephenson

Copy link
Copy Markdown
Member Author

Merge held pending human review — a protected path matched, so this PR was not merged autonomously despite being otherwise merge-ready.

The match is the "single file with more than 50 lines deleted" guard, and the file is a test file:

$ git diff --numstat origin/main...HEAD
...
59	68	src/test/java/com/preponderous/parpt/export/ProjectSorterTest.java

Sixty-eight lines were deleted from ProjectSorterTest.java, against fifty-nine inserted. The deletions are the sortByScore_* cases that no longer have a method to exercise, once ProjectSorter.sortByScore(List, boolean) was removed; their behavior was migrated onto sortBy rather than dropped, so the count of tests in that file rose from 20 to 23, and the suite total is 87 with 0 failures. No other entry on the do-not-auto-merge list matched: .github/workflows/, build.gradle and src/main/resources/application.yaml are untouched by this branch, and there is no security/ directory.

Merge-readiness otherwise:

  • ./gradlew test — BUILD SUCCESSFUL, 87 tests, 0 failures
  • CI test, build (21) and Test Results — all green on this head
  • Merge state — CLEAN / MERGEABLE
  • Self-review rubric — posted above, including an empirical stash-and-run confirmation of the one behavioral change
  • Documentation accuracy pass — README.md updated, CONTRIBUTING.md re-verified as unaffected

Confirming that the deletions above are the intended test migration is all that is needed to release the hold.

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


drafted by Claude on behalf of Daniel Stephenson

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.

Converge export sorting onto the key-based ProjectSorter.sortBy API

1 participant