feat: Convert spells units from strings to objects

[tognee]

What does this do?

Convert all hardcoded english strings to usable data. Useful for possible future localization.

-1 is used to identify infinity or an unspecified amount.

When an unit is not applicable or not specific enough a _type variable is introduced (eg. range_type, duration_type)

How was it tested?

<It's not clear if I don't update this text with relevant info>

Is there a Github issue this is resolving?

<It's not clear if I don't update this text with relevant info>

Did you update the docs in the API? Please link an associated PR if applicable.

<It's not clear if I don't update this text with relevant info>

Here's a fun image for your troubles

[fergcb]

This is definitely a step in the right direction, both for localisation and for digestibility.
Just a few things I think could make this even better; see my comments.

[fergcb]

I'm not a huge fan of -1 representing "infinity" or "until dispelled". It doesn't scan well and would be less intuitive, imho, to handle computationally. Maybe it would be more appropriate to indicate this kind of behaviour in the unit? Or, given the use of duration_type, couldn't duration simply be omitted for certain duration_types, rather than supplying data that looks valid but patterns differently to the rest?

{
  "value": 1,
  "unit": "infinity",
  "up_to": true
}

On a related note, is sec ever used anywhere besides "Instantaneous" (0 sec) and "Until dispelled" (-1 sec)? Perhaps "Instantaneous" out to be replaced with it's own unit, or a duration_type that lacks a duration, also?

[tognee]

Yeah, using duration_type where a unit is not specifiable should be better imo

[ecshreve]

I think the duration_type without a duration makes sense for the instantaneous and until something happens cases. We might want to define a model or enum in the API to make it clear to a consumer what values can be expected in those cases.

[ecshreve]

drive by review

Looks like this would add a new convention for how time durations are dealt with. Would it make sense to define a new model in the API for Duration? (similar to Cost)

EDIT: on a similar note, we might want to define a model for Distance to handle the distance/range values here

[ogregoire]

Why use abbreviations? Why is the abbreviation superior to the full name? This is a real question: I'm not for abbreviations nor against, but I'm genuinely questioning why you chose abbreviations instead of the full name?

[ogregoire]

Other question: why not use a uniform domain like this:

{
  type: feet
  amount: 1
}
{
  type: hour
  amount: 1 // or quantity or...
  qualifier: "up to" // or "or less" or "<="
}
{
  type: "until disspelled"
}

We already know it's a duration, distance, ... because of the duration, range, distance field.

[tognee]

Why use abbreviations? Why is the abbreviation superior to the full name? This is a real question: I'm not for abbreviations nor against, but I'm genuinely questioning why you chose abbreviations instead of the full name?

The convention was discussed some time ago on the discord server, I've used that.

[fergcb]

To be fair, I believe the singular/plural was only mentioned once on this discord server, and wasn't discussed any further than one suggestion from bunglero#2838.

The original reasoning by bunglero was that abbreviations don't need to be pluralised. He did also offer the solution of always pluralising. Personally, I disagree with the idea of using abbreviations to mitigate the issue of pluralisation; I'd be more inclined to forgo abbreviations in favour of full forms, either always singular (my preference) or always plural, for consistency.

This structure is designed to be easily digested by a computer, right? So human-readability should be a second priority, imo; we don't need to worry about the difference between 5 hour and 5 hours. If we really want to supply a grammatically valid English duration, perhaps it should be a separate field, such as a desc attribute on the Duration structure, which contains the source text from the SRD?

[fergcb]

Other question: why not use a uniform domain like this:
...
We already know it's a duration, distance, ... because of the duration, range, distance field.

@ogregoire This is effectively what I already suggested in one of my review comments, except renaming unit to type. Something on this pattern would remove the need for the additional _type fields.

[tognee]

If you need to convert feet and miles to meters having unit (or type of you want to rename it) take up something that isn't a measurement unit can be confusing

[fergcb]

If you need to convert feet and miles to meters having unit (or type of you want to rename it) take up something that isn't a measurement unit can be confusing

@tognee Why would someone be converting feet and miles into meters in D&D 5e? The game exclusively uses imperial unit names.

[tognee]

Why would someone be converting feet and miles into meters in D&D 5e?

I'm italian and started playing recently, the italian (and I think also all the other countries that doesn't use the imperial system) translations of the SRD uses meters as a measurement unit by multiplying feet values by 0.3

If we want to localize the database with other languages this needs to be taken in consideration

[fergcb]

If we want to localize the database with other languages this needs to be taken in consideration

If we want to localize measurements like this in the database (note that there are no official translations of the SRD to use as a basis for decisions like this), the solution is trivial; check if the unit/type is a translatable unit (e.g. foot, mile etc.), and if it is, translate it accordingly. As it stands the special units being proposed here, like until-dispelled or instantaneous, aren't measurements of distance but of time, and therefore wouldn't need to be translated this way, anyway - units of time are far more universal than those of distance.

For example, your PR already contains the action "unit".

[ogregoire]

Other question: why not use a uniform domain like this:
...
We already know it's a duration, distance, ... because of the duration, range, distance field.

@ogregoire This is effectively what I already suggested in one of my review comments, except renaming unit to type. Something on this pattern would remove the need for the additional _type fields.

No, my take on that is to use qualifier: "up to" (or something else), rather than a boolean. With your solution, we could have at_least: true and at_most: true. Those are kind of mutually exclusive in the SRD, so it's best to have an enum or at least different values grouping those mutually exclusive ideas into one.

[tognee]

If I understood the game correctly an action unit should be 6 seconds, I think that can be kept like that.
until-dispelled can't be translated as an unit because it's based on the player actions.
instantaneous can be translated to 0 seconds but I don't consider it a unit as you can't have more than 1 instantaneous.

I'm looking at the data as something that can be used to manage a game, if that's not the scope of this database then I misunderstood it.

My longterm plan is using the database to help me play games of DnD5e in my language (italian) and making data detached from english grammar can help me get there.

[fergcb]

No, my take on that is to use qualifier: "up to" (or something else), rather than a boolean. With your solution, we could have at_least: true and at_most: true.

Sure, you had two ideas going on in your suggestion. I was talking about the other, relating to the use of type.

[fergcb]

If I understood the game correctly an action unit should be 6 seconds, I think that can be kept like that.
until-dispelled can't be translated as an unit because it's based on the player actions.
instantaneous can be translated to 0 seconds but I don't consider it a unit as you can't have more than 1 instantaneous.

@tognee You're missing the point. Units of time don't have to be translated in the same way as units of distance would.

A second in English is a second in Italian, is a second in Chinese.

Unless we're translating the API into some language that measures time in flimflarks or something, introducing inconvertible "units" like instant is a non-issue.

[tognee]

I was thinking of it in a logic standpoint more than a language standpoint regarding time units

[bagelbits]

Other question: why not use a uniform domain like this:

{
  type: feet
  amount: 1
}
{
  type: hour
  amount: 1 // or quantity or...
  qualifier: "up to" // or "or less" or "<="
}
{
  type: "until disspelled"
}

We already know it's a duration, distance, ... because of the duration, range, distance field.

I really like the elegance of @ogregoire's suggestion. It also gives us a consistent shape. Which makes both GraphQL and Documentation easier.

[bagelbits]

@tognee Have you had a chance to look at this any more? It's okay if you don't time anymore.

[tognee]

Didn't had time due to university stuff. Might continue after the exams

[tognee]

I found some time to finish this pull request, now it should follow the suggested format

[SleeplessOne1917]

@SleeplessOne1917 atting myself here so I'll hopefully be notified when this merges and I can update the GraphQL API to support this change.