-
-
Notifications
You must be signed in to change notification settings - Fork 29
Fix factions order #1078
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
base: develop
Are you sure you want to change the base?
Fix factions order #1078
Conversation
WalkthroughThe Faction enum constants are reordered, changing the sequence from AEON, CYBRAN, UEF, SERAPHIM, NOMAD, UNKNOWN to UEF, AEON, CYBRAN, SERAPHIM, NOMAD, UNKNOWN. This reordering affects ordinal-based value mappings in conversion methods. Changes
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~25 minutes
Poem
Pre-merge checks and finishing touches✅ Passed checks (3 passed)
✨ Finishing touches
🧪 Generate unit tests (beta)
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Actionable comments posted: 0
🧹 Nitpick comments (1)
src/main/java/com/faforever/api/data/domain/Faction.java (1)
12-12: Enum reordering changes persisted/JSON mapping; verify external expectations and consider decoupling fromordinal()This reorder will change the mapping for all usages of
fromFaValue(int)/toFaValue()and for the JPAFactionConverter(database column values). If the old mapping was wrong and FA’s canonical values are1=UEF, 2=AEON, 3=CYBRAN, 4=SERAPHIM, 5=NOMAD, then this is the right fix—but it’s still a breaking change for any existing stored values and external clients expecting the previous behavior.I’d suggest:
- Confirm that all producers/consumers of the numeric faction value (DB data, other services, game server) agree on the new mapping
{1→UEF, 2→AEON, 3→CYBRAN, 4→SERAPHIM, 5→NOMAD}and, if needed, plan a data migration or deployment choreography.- Add explicit tests that lock in the intended mapping, e.g.:
assertEquals(UEF, fromFaValue(1)); …assertEquals(1, UEF.toFaValue()); …- Medium‑term, consider decoupling from
ordinal()by adding an explicit FA code to the enum (e.g.,UEF(1, "uef"),AEON(2, "aeon"), …) and implementingfromFaValuevia a lookup table rather thanvalues()[value - 1]. That would make future enum insertions/reorders safer and remove the “order is crucial” footgun.
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (1)
src/main/java/com/faforever/api/data/domain/Faction.java(1 hunks)
⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (2)
- GitHub Check: Codacy Static Code Analysis
- GitHub Check: test
Summary by CodeRabbit
✏️ Tip: You can customize this high-level summary in your review settings.