Naming pass: Go readability without being slaves to the terminology - #375
Merged
Conversation
Renames ix->importance (worldgen), Config/Structure->HullConfig/HullMaterial (shipgen), World->Node (route, was colliding with worldgen.World), csk/CCMode/ szPlus1D-style abbreviations (shipcombat, chargen) to descriptive names, and fixes a local-type shadow in cmd/shipgen. Genuine rulebook terms-of-art (Flux, Poice, ThisIsHardDice) are left untouched. Documents the resulting policy in CLAUDE.md so future naming decisions don't need to be re-litigated. All goldens and README-parity tests pass byte-identical with no -update, confirming the pass touched nothing observable. Co-Authored-By: Claude
…ction Renames mastermod.Table.Dice->Notation (the field holds non-parseable labels like "Bad Flux" in three known cases, so Dice wrongly promised dice.Parse compatibility), epic.Finale->Climax (an orphaned synonym for the "climax" used everywhere else in the package), and seedsearch.Find's want->matches param for clarity. Also resolves the sophont.CharName vs chargen.Characteristic question deferred from round 1: it isn't a naming inconsistency (CharName has 14 members, 8 with no chargen counterpart, and its abbreviations are the book's own chart-06A column identity) so no rename is warranted. Documents the real underlying issue instead: chargen.GenerateSophont's unchecked positional trust between species.Chars[i] and Characteristic(i). All goldens and README-parity tests remain byte-identical with no -update. Co-Authored-By: Claude
There was a problem hiding this comment.
Pull request overview
This PR performs a repo-wide naming consistency pass across Traveller5 generators and supporting packages, aligning identifiers with the documented policy that favors Go readability for ad-hoc abbreviations while preserving true rulebook terms-of-art.
Changes:
- Renames a number of parameters/types/functions to clearer Go-friendly names (e.g.
csk→attackerAsset,CCMode→ControllingCharMode,Config→HullConfig). - Updates tests and package documentation to follow the renamed identifiers.
- Codifies the naming policy in the top-level
CLAUDE.md, plus adds clarifying documentation aroundsophont.CharNamevschargen.CharacteristicandGenerateSophont’s slot-order assumption.
Reviewed changes
Copilot reviewed 44 out of 44 changed files in this pull request and generated 2 comments.
Show a summary per file
| File | Description |
|---|---|
| internal/worldgen/worlddata.go | Renames Nobility parameter from ix to importance for clarity. |
| internal/worldgen/world.go | Renames importance helper parameter (but currently introduces a shadowing name). |
| internal/worldgen/extensions.go | Renames ix parameter in RollCultural to importance. |
| internal/task/mishap.go | Renames CheckMishap → EvaluateMishap. |
| internal/task/mishap_test.go | Updates mishap test to call EvaluateMishap and updates error text. |
| internal/task/CLAUDE.md | Updates documentation to reference EvaluateMishap. |
| internal/survey/survey.go | Updates routing projection type from route.World to route.Node. |
| internal/sophont/CLAUDE.md | Documents why CharName shouldn’t be renamed; documents GenerateSophont slot-order trust. |
| internal/sophont/characteristics.go | Adds slot-order documentation to CharSpec. |
| internal/shipgen/shipgen.go | Renames hull-related types (Config → HullConfig, Structure → HullMaterial). |
| internal/shipgen/shipgen_test.go | Updates tests for renamed shipgen types. |
| internal/shipgen/hull.go | Updates hull helpers and signatures for renamed shipgen types. |
| internal/shipgen/hull_test.go | Updates hull tests to use HullConfig. |
| internal/shipgen/generate.go | Updates generators/parsers for renamed shipgen types. |
| internal/shipgen/generate_test.go | Updates generate tests for renamed shipgen types. |
| internal/shipgen/fuelarmor.go | Updates armor helpers for renamed shipgen types. |
| internal/shipgen/design_test.go | Updates design tests for renamed shipgen types. |
| internal/shipcombat/ship.go | Renames CSK-ish parameters to *Asset terminology and updates guidance helper args. |
| internal/shipcombat/explosion.go | Renames szPlus1D parameter to detonationRoll. |
| internal/shipcombat/combat.go | Renames csk to attackerAsset in targeting/resolve helpers. |
| internal/seedsearch/seedsearch.go | Renames predicate parameter want → matches and updates docs accordingly. |
| internal/route/route.go | Renames routing entity World → Node and updates Build signature. |
| internal/route/route_test.go | Updates route tests for Node rename. |
| internal/mastermod/mastermod.go | Renames Table.Dice → Notation and updates constructors/validation. |
| internal/mastermod/mastermod_test.go | Updates tests to use Notation for parseability checks. |
| internal/mastermod/CLAUDE.md | Updates documentation to reference Notation instead of Dice. |
| internal/epic/epic.go | Renames Finale → Climax and updates Adventure/Scaffold accordingly. |
| internal/chargen/spacer.go | Updates CCMode enum usage to RotatingCC. |
| internal/chargen/sophont.go | Documents the positional slot-order assumption when mapping species chars to chargen characteristics. |
| internal/chargen/soldier.go | Updates CCMode enum usage to RotatingCC. |
| internal/chargen/scout.go | Updates CCMode enum usage to RotatingCC. |
| internal/chargen/scholar.go | Updates CCMode enum usage to RotatingCC. |
| internal/chargen/rogue.go | Updates FixedCC naming to FixedControllingChar (and doc comment wording). |
| internal/chargen/rogue_test.go | Updates test comment terminology for FixedControllingChar (test name still references FixedCC). |
| internal/chargen/marine.go | Updates CCMode enum usage to RotatingCC. |
| internal/chargen/CLAUDE.md | Updates documentation to ControllingCharMode / FixedControllingChar terminology. |
| internal/chargen/career.go | Renames CCMode type/values to ControllingCharMode (RotatingCC, FixedControllingChar). |
| internal/chargen/career_test.go | Updates unit tests for renamed ControllingCharMode constants. |
| internal/chargen/agent.go | Updates CCMode enum usage to RotatingCC. |
| internal/benchmark/size.go | Renames SizeAt → Size. |
| internal/benchmark/benchmark_test.go | Updates benchmark test calls/error messages to use Size. |
| docs/rules/mastermod.md | Updates documentation to refer to Notation instead of Dice. |
| cmd/shipgen/main.go | Renames local optional wrapper types (Mount/Range) to optionalMount/optionalRange. |
| CLAUDE.md | Adds explicit repo-wide naming policy guidance. |
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
Comment on lines
+240
to
+245
| func importance(importance int) string { | ||
| if importance == 0 { | ||
| return "{0}" | ||
| } | ||
|
|
||
| return fmt.Sprintf("{%+d}", ix) | ||
| return fmt.Sprintf("{%+d}", importance) |
| @@ -291,7 +291,7 @@ func TestRogueSchemeInfamy(t *testing.T) { | |||
| } | |||
|
|
|||
| func TestRogueFixedCCChosenOnce(t *testing.T) { | |||
2 tasks
philoserf
added a commit
that referenced
this pull request
Aug 5, 2026
## Summary - Follow-up to #375. Copilot's review posted after that PR was merged and flagged two real issues, both fixed here: - `worldgen.importance`'s own parameter was named `importance`, shadowing the function itself — renamed to `value`. - `TestRogueFixedCCChosenOnce` still referenced the pre-rename `FixedCC` identifier — renamed to `TestRogueFixedControllingCharChosenOnce`. - Also proactively fixed two more test names left stale by the same pattern (`TestCheckMishap`→`TestEvaluateMishap`, `TestSizeAt`→`TestSize`), since Copilot's rationale applies equally to those. ## Test plan - [x] `task check` green - [x] `go test ./cmd/...`: goldens/READMEs byte-identical, no `-update`
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
csk→attackerAsset,CCMode→ControllingCharMode,szPlus1D→detonationRoll); genuine rulebook terms-of-art stay verbatim (Flux,Poice,ThisIsHardDice). Applies it acrossroute,shipgen,worldgen,cmd/shipgen,task,benchmark,chargen,shipcombat.internal/packages (mastermod,epic,tradecode,seedsearchwere untouched by round 1). Renamesmastermod.Table.Dice→Notation,epic.Finale→Climax,seedsearch.Find'swant→matches. Retracts round 1's deferred sophont/chargen item —sophont.CharNamevschargen.Characteristicwas never a naming inconsistency (CharName has 14 members, 8 alien-analog with no chargen counterpart, and its abbreviations are the book's own chart-06A column identity) — and instead documents the real underlying issue:chargen.GenerateSophont's unchecked positional trust betweenspecies.Chars[i]andCharacteristic(i).Test plan
task check(golangci-lint + go test) green on both commitsgo test ./cmd/...with no-update: every golden fixture andAssertReadmeUpToDatecheck passes byte-identical, confirming the renames touched nothing observable