5e-bits / 5e-database

Database for the D&D 5th Edition API
http://dnd5eapi.co/
MIT License
721 stars 354 forks source link

`Choice` structure reform #383

Closed fergcb closed 2 years ago

fergcb commented 3 years ago

Summary

There are a number of issues with the Choice structure that keep cropping up: nested Choices, arrays of mixed types, no way to easily describe or deserialize the structure... This issue aggregates those problems and proposes a reformed Choice structure that aims to solve them.

The details of the existing problems and proposed solutions are explained below. I've also described the final proposed structure in TypeScript syntax for reference (this is not intended to be official documentation), and provided an example of the structure in the form of the Fighter's starting_equipment_options array. Both can be found in this gist: https://gist.github.com/fergcb/a02760c460def3904358cbdfea9312c6

With the community's approval I'll be more than happy to take up the task of implementing the proposal, as well as producing the user-facing documentation.

Details

Consistency

The biggest issue being addressed is consistency. A lot of data consumers don't like arrays with items of mixed types (#274), and there are a variety of structures that can appear in different places without any straightforward way of identifying them.

The proposal uses an inheritance pattern to give all items in an array a common type, for easy deserialization. For example, in the from array of a Choice, there could be nested choices, arrays of items that must each be resolved separately, or a "terminal", single item. Currently these are given in their raw forms; the from array could contain Choice objects, JSON arrays, or single objects.

To solve this, every item in the array will now be an object with an option_type attribute of either choice, multiple or single, which indicates the structure of the underlying data. The same pattern is used to identify whether the Choice is between an array, as described above, or a ResourceList. See the JSON snippet in the gist for examples.

Descriptions

Another issue with the current Choice structure is that it can be hard to explain to a human user what they're actually choosing between (#355). While it is easy to list the options available, and the type attribute of a choice provides some information, a consumer has no way of indicating where the choices are coming from, or what they are. For example type may indicate that a "proficiency" is being chosen, so an application could easily render something like "Choose one proficiency:", but the underlying choice in the SRD is actually "one type of artisan's tools or one musical instrument"

To solve this, the proposal adds an optional desc string for Choices, so that consumers don't have to write any funky algorithms to tell a human user what they're choosing from. The string should include the text from the SRD that describes what needs to be chosen, e.g. "a wooden shield or any simple weapon", "one type of artisan's tools or one musical instrument", "a martial weapon and a shield or two martial weapons", etc.

Amendments

Final Spec

This section describes the state of the proposal after the discussed amendments, to aid with the implementation:

Choice

The Choice structure describes a decision that must be made by a player during character creation or gameplay. It has four attributes:

OptionSet

The OptionSet structure provides the options to be chosen from, or sufficient data to fetch and interpret the options. All OptionSets have an option_set_type attribute that indicates the structure of the object that contains the options. The possible values are options_array, equipment_category and reference_list. Other attributes on the OptionSet depend on the value of this attribute.

Option

When the options are given in an options_array, each item in the array inherits from the Option structure. All Options have an option_type attribute that indicates the structure of the option. The possible values are reference, action, multiple and choice. The value of this attribute indicates how the option should be handled, and each type has different attributes.

Wyzards commented 3 years ago

I already said this on discord but am saying it again for github users..

I approve of the structure. My main concern is how we would document this. I am sure it can be done properly it's just something we will have to be careful with.

Additionally I think it would be best to have more specific type names in the data. The type field for OptionSets could be named option_set_type and the type field for Option could be named option_type. I think the actual values for these fields are named sufficiently.

Good luck.

Wyzards commented 3 years ago

Additionally now that I'm thinking about it, it could also be good to use a regular APIReference as an Option as well. It seems odd to have certain things (like spells, sub-features and sub-traits) to be counted, even if it's just 1. Perhaps this isn't needed though.

fergcb commented 3 years ago

Additionally I think it would be best to have more specific type names in the data. The type field for OptionSets could be named option_set_type and the type field for Option could be named option_type. I think the actual values for these fields are named sufficiently.

I can see this being beneficial. If others agree I'd be happy to go with this.

now that I'm thinking about it, it could also be good to use a regular APIReference as an Option as well. It seems odd to have certain things (like spells, sub-features and sub-traits) to be counted, even if it's just 1. Perhaps this isn't needed though.

I had thought about this, and cut it out in an effort to simplify a little, with the justification that a count of 1 does the trick, but that is quite StartingEquipment-centric thinking from me. There are other places that do require counts, like actions (esp. Multiattack actions), but those that don't probably outweigh them.

Requiring count across the board works, but might not be as intuitive. Introducing an uncounted APIReference would also work, might be more intuitive, but would increase complexity. I'm interested to know what others have to say about this.

ogregoire commented 3 years ago

Globally, I like this structure, especially the desc suggestion, but I believe names should be better.

Option

type Option = {
  type: 'single' | 'array' | 'choice'
  prerequisites?: Prerequisite[]
} & (SingleOption | ArrayOption | ChoiceOption)

I don't like the word array: it's too JSON-y and not data enough. In regards to data, I believe the term list is preferred. When it comes to a choice between choice, array and single, it's very weird to use array here. Can we use choice, multiple, single, or maybe one_of, all_of, one? I'm not a native English speaker but to my ear as a developer for so many years, this sounds very strange. So maybe other words can be preferred. Any suggestion?

OptionSet

type OptionSet = {
  type: 'resource_list' | 'array'
} & (ResourceListOptionSet | ArrayOptionSet)

This really requires improvement:

Ok, I understand that some people will invalidate my first point with my second or vice-versa, but the point here is to be self-contained relevancy. And OptionSet can actually contain a reference to options, or options themselves. Hence my suggestion.

Yes, I know, naming is hard. Sorry...

fergcb commented 3 years ago

I don't like the word array: it's too JSON-y

The whole point of the attribute is to describe the structure of the underlying data. That data is a JSON array; hence, the use of JSON terminology.

Certainly choice, multiple, single is far better than one_of, all_of, one, as it much closer to describing the actual structures here. For example, one_of implies an array of options, one of which must be selected, when in actual fact, it corresponds to a Choice object. Once again, I am interested to hear what others have to say on this.

the resource_list is not a list of resource: it's a reference to a resource list. Can we just use reference instead of resource_list?

I would be happy to use resource_list_reference, or similar. As you're correct, it's not a ResourceList, but a reference to one. I feel reference alone is too vague.

the .ts should be there to explain the JSON, not to enforce it.

The entire purpose of TS types is to enforce the type, or structure, of data at compile time. While it may help explain the structure for those who understand the syntax, it is chiefly here to allow the schema to be enforced while it is being implemented.

the array here makes little sense as seen here. I suggest we use the name options instead of array.

The solution to this is not to give the attribute a value of options. That does not describe the structure of the data. The from attribute of the Choice structure contains the options. The type attribute of the OptionSet structure indicates the format in which those options are presented: Either a ResourceList, or a JSON Array.

I believe Wyzards' suggestion of naming the attributes option_set_type and option_type is a far stronger suggestion, as it actually corresponds to the purpose of the attributes, rather than trying to give them a different, less useful purpose altogether.

ETA: as I have mentioned in previous discussions, the value of the type attribute IS NOT a name. Treating it as one is counter-productive.

ogregoire commented 3 years ago

The whole point of the attribute is to describe the structure of the underlying data. That data is a JSON array; hence, the use of JSON terminology.

No, we can't just go around and name everything array. If I have a list of bikes in a structure and I name that list of bikes list, my reviewer will reject my code telling me to call it bikes or similar. An array is a good structure, but array is a bad name for something that uses that structure. I don't really care if it's the one_of and friends that's selected or multiple, but array has to leave, here.

The entire purpose of TS types is to enforce the type, or structure, of data at compile time. While it may help explain the structure for those who understand the syntax, it is chiefly here to allow the schema to be enforced while it is being implemented.

I understand why TS is here, but I don't work in JavaScript and friends. I work in different languages such as Java and Python, and if at some point in time, TypeScript goes against JSON and creates its own TSON (for instance), your stance means that you'll think about follow it, making the data secondary to the structure rather than primary as it (still) currently is. I must say that if the data is not in pure JSON, I'm out, forking the data of this project.

So yes, it's important to understand that JSON is king, and TS is only here to support it.

The solution to this is not to give the attribute a value of options. That does not describe the structure of the data.

It's not about describing the structure of the data: it's about describing the data. array is a bad name and I'll reject it. I'm fine with you not liking options, but let's find something else that is not options or array in that case. We can try going with option_set_type and option_type, as you mention: I'm fine with testing it at the very least.

fergcb commented 3 years ago

It's not about describing the structure of the data: it's about describing the data.

This is were we disagree. As I keep saying, the type attribute only describes the structure of the data. That's it.

Please stop getting hung up on the TS thing. It will not be included in the project anywhere. It is only used by me to validate the data as it is being entered. I only included it for people who do understand it, in the hope it may assist the explanation. You don't even have to lay eyes on it if you don't want to.

I feel like almost all of your comments stem from a fundamental misunderstanding of the issues and the proposed solutions, and I don't know how I can explain them in ways other than those I already have. I'm going to reserve further comment until others have had a chance to take a look at the proposal and the discussion, in the hope that different perspectives can shed some light on this blip in communication.

ogregoire commented 3 years ago

I do understand your proposals. Structurally your suggestion is excellent. I really like it. Please just try to read the JSON you wrote (lines 61-72):

  {
    "desc": "(a) a martial weapon and a shield or (b) two martial weapons",
    "type": "equipment",
    "choose": 1,
    "from": {
      "type": "array",
      "options": [
        {
          "type": "array",
          "items": [
            {
              "type": "choice",

What I read is: "my type is array, but I'm offering options as field".

Then the very next object reads "my type is array, but I'm offering items as field".

So because you prefer to describe the structure of the data and not the data itself, there is a huge confusion here: at some point an object that says it's an array contains options and at some other time, another object that says it's an array contains items. How is that not confusing? This is what I have an issue with.

All the rest is fine by me, and I mean all.

Wyzards commented 3 years ago

I don't like the word array: it's too JSON-y

The whole point of the attribute is to describe the structure of the underlying data. That data is a JSON array; hence, the use of JSON terminology.

Certainly choice, multiple, single is far better than one_of, all_of, one, as it much closer to describing the actual structures here. For example, one_of implies an array of options, one of which must be selected, when in actual fact, it corresponds to a Choice object. Once again, I am interested to hear what others have to say on this.

the resource_list is not a list of resource: it's a reference to a resource list. Can we just use reference instead of resource_list?

I would be happy to use resource_list_reference, or similar. As you're correct, it's not a ResourceList, but a reference to one. I feel reference alone is too vague.

the .ts should be there to explain the JSON, not to enforce it.

The entire purpose of TS types is to enforce the type, or structure, of data at compile time. While it may help explain the structure for those who understand the syntax, it is chiefly here to allow the schema to be enforced while it is being implemented.

the array here makes little sense as seen here. I suggest we use the name options instead of array.

The solution to this is not to give the attribute a value of options. That does not describe the structure of the data. The from attribute of the Choice structure contains the options. The type attribute of the OptionSet structure indicates the format in which those options are presented: Either a ResourceList, or a JSON Array.

I believe Wyzards' suggestion of naming the attributes option_set_type and option_type is a far stronger suggestion, as it actually corresponds to the purpose of the attributes, rather than trying to give them a different, less useful purpose altogether.

ETA: as I have mentioned in previous discussions, the value of the type attribute IS NOT a name. Treating it as one is counter-productive.

I think the choice, multiple, single name scheme is best. Also how about resource_list_url for the resource list url lol... I agree that reference would be too ambiguous.

Also I agree with what ferg is saying about the array naming. The type field is describing the literal type structure of the choice, so array and resource list seems most fitting since it's not actually describing the content of the optionset itself. The type field exists here to distinguish from the two OptionSets type and tells the consumer how to handle them. It isn't indicating what type of data is stored within the choice. When it comes down to it I'm not too hung up on whether array is renamed to options or not, I think either is properly indicative of what is stored within it.

If the type field were meant to describe the type of CONTENTS, then I would agree with @ogregoire but as far as I can tell that is not the case.

Wyzards commented 3 years ago

Was just taking a look again at the gist and noticed that your schema for Option has an attribute called prerequisites. It's clear to me what the purpose of this could be, but could you provide an example that makes use of it within the SRD?

fergcb commented 3 years ago

Was just taking a look again at the gist and noticed that your schema for Option has an attribute called prerequisites. It's clear to me what the purpose of this could be, but could you provide an example that makes use of it within the SRD?

Cleric gets "(a) a mace or (b) a Warhammer (if proficient)".

You can see how this is represented in the API currently using the prerequisites attribute:

https://dnd5eapi.co/api/classes/cleric/starting-equipment

Wyzards commented 3 years ago

Was just taking a look again at the gist and noticed that your schema for Option has an attribute called prerequisites. It's clear to me what the purpose of this could be, but could you provide an example that makes use of it within the SRD?

Cleric gets "(a) a mace or (b) a Warhammer (if proficient)".

You can see how this is represented in the API currently using the prerequisites attribute:

https://dnd5eapi.co/api/classes/cleric/starting-equipment

I see, thanks. That makes sense.

Now I'm just waiting for these changes to be implemented lol, I've already inserted the data structure into my own code since it's sort of all encompassing.

fergcb commented 3 years ago

Now I'm just waiting for these changes to be implemented

As soon as I have agreement from the maintainers, I'll get to work.

bagelbits commented 3 years ago

Haha. I feel reasonably called out and I definitely deserve it. @fergcb I have no plans tonight after work so I'll dedicate it to this.

bagelbits commented 3 years ago

I agree with @Wyzards that type as a field name feels a bit overloaded and it has a different meaning depending on where in the structure you are. This could become quite confusing.

I agree with @ogregoire that OptionSet should probably not use the term resource_list. Perhaps api_reference instead of reference? That seems a little less vague. I kind of feel like resource_list_url isn't quite right either. And makes sense to use our ApiReference structure that we already have?

I also like choice, multiple, single as the options. I think that's a bit clearer.

@fergcb If it's not too much of a bother, can you update that gist with the changes we've agreed on so far? Just so it's easier to track where we are now that we've agreed on some things?

bagelbits commented 3 years ago

I also feel like the overloading of our usage of the type field might have been causing some confusion in the discussion.

That being said, just a friendly remind to try and keep discussions friendly and civil. We're all doing this for fun. Or at least... I'm doing this for fun.

Wyzards commented 3 years ago

That being said, just a friendly remind to try and keep discussions friendly and civil. We're all doing this for fun. Or at least... I'm doing this for fun.

This API is for blood, and blood only. No fun.

@fergcb If it's not too much of a bother, can you update that gist with the changes we've agreed on so far? Just so it's easier to track where we are now that we've agreed on some things?

In all seriousness though, what you said here would be great and make discussing this a lot more clear.

bagelbits commented 3 years ago

This API is for blood, and blood only. No fun.

Blood for the blood god. Skulls for the skull throne.

fergcb commented 3 years ago

OptionSet should probably not use the term resource_list. Perhaps api_reference instead of reference? That seems a little less vague. I kind of feel like resource_list_url isn't quite right either. And makes sense to use our ApiReference structure that we already have?

@bagelbits the data corresponding to the type resource_list isn't an APIReference, and it can't be represented that way.

We already have the ResourceList structure, and that's that the URL links to. A ResourceList doesn't have an index or a name, just a URL.

I'm fine with all the other changes though :)

bagelbits commented 3 years ago

Okay! Then resource_list_url probably makes sense.

fergcb commented 3 years ago

The gist has been updated to reflect the following suggested amendments:

(@bagelbits @Wyzards)

Wyzards commented 3 years ago

That seems a little less vague. I kind of feel like resource_list_url isn't quite right either.

When I suggested resource_list_url I was referring to the attribute that stored the resource list url, not the name for the OptionSet type.

In that respect I thought the names array and resource_list were sufficiently descriptive. Now that my suggestion has been implemented in the wrong place its less accurate. @fergcb

fergcb commented 3 years ago

When I suggested resource_list_url I was referring to the attribute that stored the resource list url, not the name for the OptionSet type.

@Wyzards this change was based more on @bagelbits suggestion later in the thread. I think changing the value of the option_set_type makes the most sense, because it's describing the structure of the data within, and the data is a URL, not a ResourceList. The url attribute could be renamed as well, but it wouldn't be as useful of a change by itself.

Wyzards commented 3 years ago

When I suggested resource_list_url I was referring to the attribute that stored the resource list url, not the name for the OptionSet type.

@Wyzards this change was based more on @bagelbits suggestion later in the thread. I think changing the value of the option_set_type makes the most sense, because it's describing the structure of the data within, and the data is a URL, not a ResourceList. The url attribute could be renamed as well, but it wouldn't be as useful of a change by itself.

Agree to disagree I suppose. Doesn't make too much of a difference to me in the long run.

ogregoire commented 3 years ago

Okay! Then resource_list_url probably makes sense.

No it doesn't.

I will translate that name exactly as it means:

Resource List Uniform Resource Locator

Why does anyone want to use twice the word "resource" in a single name? That makes no sense.

I strongly advise to use a different name that makes actual sense and that doesn't repeat.

fergcb commented 3 years ago

Why does anyone want to use twice the word "resource" in a single name? That makes no sense.

I strongly advise to use a different name that makes actual sense.

The data the value is describing is a URL. That URL resolves to a ResourceList. Hence, resource_list_url.

Perhaps suggesting a suitable alternative would be a more productive contribution to the discussion?

ogregoire commented 3 years ago

Perhaps suggesting a suitable alternative would be a more productive contribution to the discussion?

I did, but you rejected it. At least in the sense that I suggested alternatives to resource list, which is the root cause of the issue.

All those lists are not resource lists. They're item lists, spell lists, etc. Using the global name resource for those is hype-y and not descriptive. Maybe we should use a global name such as game element for those, as they're part of the game and it's a fitting name to describe what both equipment items and spells are. And then we could have something akin to "game_element_list_url", or "element_list_reference". And I insist that reference is adequate (and better than URL) because it's a pointer, it describe that the list refers to something that is described elsewhere. The reference happen to be a URL, but it's still a reference. So my prime candidate for naming that field is "element list reference". What do you think of this? Oh, and please stop saying I don't suggest alternatives when in fact I did.

fergcb commented 3 years ago

I did, but you rejected it.

Are you referring to your suggestion of "reference"? I hope it's clear after the following discussion why that isn't really an appropriate alternative.

Maybe resource list is not a good name then?

Maybe. I feel that's distracting from the goal of this proposal. If we decide to change the documented name of that structure, we can always revisit the use of its name in this structure. If you think it's truly that in-need of an update, you could open an issue about it on the API repo (where the docs live)?

Wyzards commented 3 years ago

I did, but you rejected it.

Are you referring to your suggestion of "reference"? I hope it's clear after the following discussion why that isn't really an appropriate alternative.

Maybe resource list is not a good name then?

Maybe. I feel that's distracting from the goal of this proposal. If we decide to change the documented name of that structure, we can always revisit the use of its name in this structure. If you think it's truly that in-need of an update, you could open an issue about it on the API repo?

I still think that resource_list is the most acceptable name. reference isn't sufficient because it points to a resource list, which is established in the documentation at https://www.dnd5eapi.co/docs/#resource-lists.

The option_set_type of the OptionSet essential refers to how the data is stored. In the case of the one that uses a resource list, I think that resource_list subsequently makes the most sense...

Same case for the array OptionSet type.

I'll also note that I agree, this is distracting from the goal of the proposal and could always be changed later.

fergcb commented 3 years ago

I still think that resource_list is the most acceptable name. reference isn't sufficient because it points to a resource list, which is established in the documentation at https://www.dnd5eapi.co/docs/#resource-lists.

I'd be happy to revert to resource_list, my original proposal. That particular change was in response to @bagelbits' suggestion, so I'd be interested to hear their thoughts on the above few comments.

bagelbits commented 3 years ago

I still think that resource_list is the most acceptable name. reference isn't sufficient because it points to a resource list, which is established in the documentation at https://www.dnd5eapi.co/docs/#resource-lists.

That's... not exactly true. The resource_list you're referring to in the docs is for an index on an endpoint and is different shape than what we're calling a resource_list here, i.e. /api/equipment-categories/martial-weapons. I think @ogregoire has a point that resource_list_url doesn't quite feel like the right fit for what we're trying to convey. But I'm having a devil of a time thinking of something that works. Let me workshop this a little.

So this is how it stands right now:

"from": {
  "option_set_type": "resource_list_url",
  "url": "/api/equipment-categories/martial-weapons"
}

And previously we had this which also didn't feel quite right:

"from": {
  "option_set_type": "resource_list",
  "url": "/api/equipment-categories/martial-weapons"
}

Hrmmm.... my first thought is doing reference_list:

"from": {
  "option_set_type": "reference_list",
  "url": "/api/equipment-categories/martial-weapons"
}

If we wanted to be a little more verbose, we could even do:

"from": {
  "option_set_type": "reference_list",
  "reference_list": {
    "name": "Martial Weapons",
    "index": "martial-weapons",
    "url": "/api/equipment-categories/martial-weapons"
  }
}

Though, honestly, that feels a little weird because reference_list makes you think it's going to be a list of things but you're only giving an object. What do y'all think?

It seems like everything else is mostly solidified though and this is our last piece. I think my only other question is what do we gain by differentiating between single and multiple?

Wyzards commented 3 years ago

I still think that resource_list is the most acceptable name. reference isn't sufficient because it points to a resource list, which is established in the documentation at https://www.dnd5eapi.co/docs/#resource-lists.

That's... not exactly true. The resource_list you're referring to in the docs is for an index on an endpoint and is different shape than what we're calling a resource_list here, i.e. /api/equipment-categories/martial-weapons. I think @ogregoire has a point that resource_list_url doesn't quite feel like the right fit for what we're trying to convey. But I'm having a devil of a time thinking of something that works. Let me workshop this a little.

So this is how it stands right now:

"from": {
  "option_set_type": "resource_list_url",
  "url": "/api/equipment-categories/martial-weapons"
}

And previously we had this which also didn't feel quite right:

"from": {
  "option_set_type": "resource_list",
  "url": "/api/equipment-categories/martial-weapons"
}

Hrmmm.... my first thought is doing reference_list:

"from": {
  "option_set_type": "reference_list",
  "url": "/api/equipment-categories/martial-weapons"
}

If we wanted to be a little more verbose, we could even do:

"from": {
  "option_set_type": "reference_list",
  "reference_list": {
    "name": "Martial Weapons",
    "index": "martial-weapons",
    "url": "/api/equipment-categories/martial-weapons"
  }
}

Though, honestly, that feels a little weird because reference_list makes you think it's going to be a list of things but you're only giving an object. What do y'all think?

It seems like everything else is mostly solidified though and this is our last piece. I think my only other question is what do we gain by differentiating between single and multiple?

I thought of reference_list as well but thought it might not fit, since the documentation already names the structure "resource list" in addition to it including the count attribute, making it not literally just a list of references, as the name would indicate.

On the final note, I'm pretty sure we already agreed that there should be a distinction between single and multiple. I assume here you're referring to the three option types, single, multiple, and choice. I feel that these should be unchanged.

Also I think just the URL is fine rather than an entire reference to the resource list, especially since the endpoint already contains information that would be stored within said APIReference.

Wyzards commented 3 years ago

I thought of reference_list as well but thought it might not fit, since the documentation already names the structure "resource list" in addition to it including the count attribute, making it not literally just a list of references, as the name would indicate.

To clarify here, I have no problem with using reference_list here, although I don't see it as much more descriptive than resource_list`

bagelbits commented 3 years ago

I thought of reference_list as well but thought it might not fit, since the documentation already names the structure "resource list" in addition to it including the count attribute, making it not literally just a list of references, as the name would indicate.

Just to be clear, /api/equipment-categories/martial-weapons is not a resource list. If we look at the docs for equipment categories, it's just index, name, and a list of APIReference. Do we have any other examples of how we would use this field besides links to Equipment Categories? I think that might help a little with how we think about this specific case.

Wyzards commented 3 years ago

I thought of reference_list as well but thought it might not fit, since the documentation already names the structure "resource list" in addition to it including the count attribute, making it not literally just a list of references, as the name would indicate.

Just to be clear, /api/equipment-categories/martial-weapons is not a resource list. If we look at the docs for equipment categories, it's just index, name, and a list of APIReference. Do we have any other examples of how we would use this field besides links to Equipment Categories? I think that might help a little with how we think about this specific case.

You're correct, this is not a resource list, that's my mistake. If starting equipment is the only application of the resource_list option-type, then resource_list would not be appropriate.

I looked a little through the SRD to find any other places where a Choice option or resource list OptionSet would need to be used, but couldn't find any. However, my search was by no means thorough and it doesn't seem completely unlikely that there would have to be a nested choice that actually does refer to a resource list.

If starting equipment is the only usage of this OptionSet type, then I think reference_list would actually be the most appropriate.

fergcb commented 3 years ago

I don't know how I missed that an equipment category endpoint doesn't return a ResourceList... That complicates things somewhat, and it's completely on me for not checking 😬

I don't feel then that something like reference_list would be appropriate, given that resolving the value doesn't give you a list of references, or a generic structure that would be easy to extract a list of references from. The options are under the equipment attribute of the returned document, which is obviously very specific to equipment categories. Even if we named the resource_list type more appropriately, it'd still be unique to equipment categories.

This makes me feel that it would be worth going even more specific and creating a new option_set_type called equipment_category? And we could use a full APIReference like bagelbits suggests. Going this specific increases the complexity for consumers a little bit, but starting equipment is probably one of the most important places where Choices are used, so I feel it would be acceptable.

My thinking behind resource_list was that it could be used to include query params in the URL to filter a ResourceList. For example, the Choice in the High Elf's wizard cantrip trait could be from the ResourceList returned by /api/spells?level=0&class=wizard.

Of course, at the time I was assuming that equipment categories returned ResourceLists, which we've now learned is wrong, so that approach is now less generalisable. But I think there may be some value in introducing it alongside a equipment-category type.

Wyzards commented 3 years ago

I don't know how I missed that an equipment category endpoint doesn't return a ResourceList... That complicates things somewhat, and it's completely on me for not checking 😬

I don't feel then that something like reference_list would be appropriate, given that resolving the value doesn't give you a list of references, or a generic structure that would be easy to extract a list of references from. The options are under the equipment attribute of the returned document, which is obviously very specific to equipment categories. Even if we named the resource_list type more appropriately, it'd still be unique to equipment categories.

This makes me feel that it would be worth going even more specific and creating a new option_set_type called equipment_category? And we could use a full APIReference like bagelbits suggests. Going this specific increases the complexity for consumers a little bit, but starting equipment is probably one of the most important places where Choices are used, so I feel it would be acceptable.

My thinking behind resource_list was that it could be used to include query params in the URL to filter a ResourceList. For example, the Choice in the High Elf's wizard cantrip trait could be from the ResourceList returned by /api/spells?level=0&class=wizard.

Of course, at the time I was assuming that equipment categories returned ResourceLists, which we've now learned is wrong, so that approach is now less generalisable. But I think there may be some value in introducing it alongside a equipment-category type.

I have no objections to this.

bagelbits commented 3 years ago

@fergcb I'd love to see an example of where we actually use a resource list as well. Also, how does this structure work with other places we use choices, like monsters for example?

fergcb commented 3 years ago

I'd love to see an example of where we actually use a resource list as well.

@bagelbits Currently, we don't. Like I explained in my previous comment, I was initially working under the assumption that equipment categories returned ResourceLists, but under the realisation that they don't, I think there still could be value in supporting them as a source of Options for a Choice; we could use this for choosing languages (where we could just refer to the /api/languages ResourceList, rather than listing every single language as APIReferences), or the high elf cantrip example I gave earlier (/api/spells?level=0&class=wizard [although now I think of it, I'm not sure the class query param exists on the spells endpoint - maybe it should]).

Also, how does this structure work with other places we use choices, like monsters for example?

This raises a good point that not all things being chosen from are references to other documents, though I think Monsters' attacks are unique in that regard? Class and Background StartingEquipment, feature/trait choices, proficiency choices, and spell choices, etc. all only use APIReferences (and therefore work the exact same way as the StartingEquipment example given), so I must have skimmed over multiattacks being different.

To handle Monsters' multiattacks, we'll either have to split the single Option type into something like reference and attack, or change the way monster attacks work. The former seems favourable.

bagelbits commented 3 years ago

I would love to see some examples of that then! And we might want to scour the rest of our choice usage to make sure we aren't missing any other edge cases. 😄

fergcb commented 3 years ago

Apologies for the inactivity; it's a busy time for me rn.

I've updated the schema in the following ways:

Wyzards commented 3 years ago

@fergcb just got the chance to look at your changes.

Obviously I agree with the first change. I also agree with equipment_category being its own OptionSet. As for splitting single Options, there's a couple nitpicks I have but overall I agree with the idea.

First of all, I feel that the Option type currently named reference should stay as single. It just seems more intuitive to me personally. Additionally, the top-level options attribute within the Multiattack Action you provided should be named something along the lines of choice, action_choice or attack_choice, since the attribute contains a choice, not a list of options. Finally, I think it could make more sense for the new option's type to be "attack" instead of "action". I can't really see this new option being used outside of attacks. If I'm wrong, please provide an example to correct me. If that is the case, then "action" is an appropriate name.

Otherwise, I think this newest rendition looks great.

fergcb commented 3 years ago

First of all, I feel that the Option type currently named reference should stay as single.

That feels misleading, as actions may also be "single" items in the choice.

the top-level options attribute within the Multiattack Action you provided should be named something along the lines of choice, action_choice or attack_choice, since the attribute contains a choice, not a list of options.

I haven't changed/added anything, there. That attribute already exists in action objects. Changing that is unrelated to the Choice structure itself and belongs in a separate issue.

I think it could make more sense for the new option's type to be "attack" instead of "action". I can't really see this new option being used outside of attacks. If I'm wrong, please provide an example to correct me. If that is the case, then "action" is an appropriate name.

Dragons use their Frightful Presence action as part of their multiattack:

Multiattack. The dragon can use its Frightful Presence. It then makes three attacks: one with its bite and two with its claws.

bagelbits commented 3 years ago

I think it could make more sense for the new option's type to be "attack" instead of "action". I can't really see this new option being used outside of attacks. If I'm wrong, please provide an example to correct me. If that is the case, then "action" is an appropriate name.

Additionally, this structure will get reused for Legendary Actions, which are currently not broken out. I think there's an open issue for it.

bagelbits commented 3 years ago

IIRC how Legendary Actions work.

Wyzards commented 3 years ago

If that's the case, then I have no other issues with this proposal.

bagelbits commented 3 years ago

I'll try to review this again later tonight so we can get cracking on this and unblock everyone.

bagelbits commented 3 years ago

I think that looks good. @ogregoire do you have any additional feedback before we try and roll this out?

ogregoire commented 3 years ago

I still dislike the word array. The rest seems to have been properly iterated upon.

We use resource_list which indicates a string but refers to some other data which are resource lists. So that's fine. We use equipment_category which indicates an API reference that refers to some other data which are equipment categories. So that's fine.

Then we use array which indicates an array of options but the type just calls it array. It's not an array, it's an array of options. So that's not fine.

We have two elements that are defined by what they will contain, and one that is defined by its json-type. That's inconsistent. If we want consistency, but keep array, we could rename the other two string and object, respectively. (Please don't, that was for the purpose of the argument.) I don't see why the work is good for two out of three, but for the third, we just stop caring about proper naming.

For the rest, I totally agree. It's a clear improvement: I can read the JSON and understand it without checking the type definitions.

bagelbits commented 3 years ago

@ogregoire Keeping with the theme. Would it make more sense to call it options instead of array?