Skip to content

[QA / FEATURE] Consolide CR fields into single "challenge_rating" field#910

Open
calumbell wants to merge 1 commit intoopen5e:stagingfrom
calumbell:902/cr-name-change
Open

[QA / FEATURE] Consolide CR fields into single "challenge_rating" field#910
calumbell wants to merge 1 commit intoopen5e:stagingfrom
calumbell:902/cr-name-change

Conversation

@calumbell
Copy link
Copy Markdown
Contributor

@calumbell calumbell commented Apr 9, 2026

Description

This PR slightly streamlines the Creature model by replacing the "challenge_rating_decimal" and "challenge_rating_text" fields with a single "challenge_rating" field.

The motivation for this was to improve the developer experience working when working with these fields, although one could argue that there are marginal performance gains to be made here.

N.B. This will break the front-end unless it is also patched to consume the "challenge_rating" field. A PR is on its way that will fix this.

Related Issue

Closes #902

How was this tested?

  • Spot checked on local Django development server
  • pytest on local (all testing passing after updating cases to reflect changes in API response)
  • CI/CD passing

@eepMoody
Copy link
Copy Markdown
Collaborator

So I do think we split these on purpose, so we could use one for sorting and another for display, if I recall.

Are we thinking it makes sense to roll it back at this point? Should we maybe synthesize one from the other or set one of them as "challenge_rating" for ease of use?

@calumbell
Copy link
Copy Markdown
Contributor Author

calumbell commented Apr 11, 2026

@eepMoody Not sure I was privy to the conversations about adding two fields to handle CR, so apologies if I have run rough-shod someone else's careful planning. But to me it makes sense to only use one field to handle CR, especially as the only difference between the data presented in "challenge_rating_decimal" and "challenge_rating_text" is that 0.5, 0.25 and 0.125 are parsed as 1/2, 1/4 and 1/8.

The reasons I think we should roll it back are:

  • Separation of concerns. The only difference between the two CR fields is formatting 0.5, 0.25 and 0.125 as fractions. Formatting ought to be handled on the front-end, with the API presenting the data as it is. Adding API bloat instead of using a three row look-up table on the front-end has the whiff of smelly code.
  • Rubbish field names. "challenge_rating" is good, intuitive name for this field. The "_text" and "_decimal" postfixes torpedo this.
  • Single source of truth. In my experience, debugging front-end code that mixes "challenge_rating_decimal" and "challenge_rating_text" fields is considerably more time-consuming than debugging code that uses the decimal representation and calls the parseChallengeRating() helper function inline to generate the textual representation inside our templates.

That is my thinking. The only hill that I die on is that the numerical CR data should live in a field called "challenge_rating", not "challenge_rating_decimal", and that we should do away with the some of the excessive trailing zeroes.

Let me know your thoughts, happy to make any additional changes required :)

@eepMoody
Copy link
Copy Markdown
Collaborator

Okay yeah, that seems reasonable. I guess we just over-engineered the CR stuff. And yeah, no need for excess trailing zeroes, nobody is tracking CR to three significant digits 🤣

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.

[FEAT REQ] Consolidate challenge_rating_text and challenge_rating_decimal into single field

2 participants