Feature trait choice #378
Feature trait choice #378
Conversation
|
This PR does too many things, and it's unclear what the goal is. Also, it seriously look like you've taken a really old version of the data and worked on that. I mean that the "Choose: " part was removed ages ago, and yet you want to include it in this PR. Is it intentional? A mistake? Please take our main branch, branch it, and cherry pick the commits you really want to merge here. |
From your comment I'm going to assume you are unaware of the discussion between myself and @fergcb where we spoke about some issues with the choice attribute and collectively decided that this is how we'd resolve them. Hence all the chances in one pr. The discussion is in the api discord. As for including Choose: in the name of some traits and features. This was following the format of existing features within the API that require you to make a choice between several subfeatures (ex: bard expertise). This is the case despite the "Choose" tag not being present in the SRD, so I continued the naming scheme. The data I have added here is accurate, although majority of the information is unchanged. |
|
While he may not be aware of the discussion surrounding this PR, @ogregoire is right in one regard; there appear to be several commits in this PR from old changes that don't relate to the changes we want, including a couple of merge commits. It would be cool if you could cherry-pick only the relevant commits and resubmit the PR! That'd make it easier to review. As for the "Choose: " thing, I don't believe we discussed this, but I'm in favour of ditching it, so that the feature/trait names are closer to the SRD. That could be a job for a separate PR. |
|
Of course I am unaware of your discussion if you don't put a link to where it happened. Discord allows linking to specific discussions, so if that discussion happened on Discord, please link to it. On that topic, @bagelbits could you setup the discussion feature to GitHub? It's only a matter of ticking a box in the settings. Discussions happening somewhere else are very disturbing, especially when it's on a platform known for its troubles on Linux. |
Ah I didn't realize he was referring to those previous commits. Those come down to my inexperience with github, and I agree they shouldn't be included. Just wasn't sure how to get them out. Will make sure to cherry pick in final PR. As for the "Choose:" naming scheme I'm indifferent. Again, I just followed the existing naming scheme. It'd likely be better to remove it so it more closely matches the SRD. I'll take care of that in a different PR so I don't put any more clutter into this one. |
https://discord.com/channels/656547667601653787/656547667601653790/861810226541625395 |
I'd actually prefer that you do it in this PR. We can squash your commits into one to remove the clutter. |
If that's what you'd prefer then I have no issues complying. |
|
@ogregoire I've turned on discussion for both DB and API. |
| "subraces": [], | ||
| "name": "Draconic Ancestry (Black)", | ||
| "desc": [ | ||
| "You have draconic ancestry. Choose one type of dragon from the Draconic Ancestry table. Your breath weapon and damage resistance are determined by the dragon type, as shown in the table." |
There was a problem hiding this comment.
Should we be showing the table somewhere? We're also missing breath weapon information?
There was a problem hiding this comment.
Underneath the subtrait_options under the trait_specific attribute of is a choice which includes all possible draconic ancestry features you can select. This is the only equivalent of the table we need imo.
As for not including breath weapon information underneath trait_specific I did this because it's less concrete. With damage_type and save_ability_scote these API references could actually be passed onto consumers and used to either deal damage or have someone make a saving throw, whereas any breath weapon information would either go unused or have to to individually interpreted. It COULD be added, nontheless.
There was a problem hiding this comment.
As for not including breath weapon information underneath trait_specific I did this because it's less concrete. With damage_type and save_ability_scote these API references could actually be passed onto consumers and used to either deal damage or have someone make a saving throw, whereas any breath weapon information would either go unused or have to to individually interpreted. It COULD be added, nontheless.
It should be added. Otherwise that data is just completely missing from the API. It could be as simple as:
"breath_weapon": {
"shape": "cone",
"size: 15,
}Substituting the values as appropriate, of course. Alternatively, it could be formatted more like the actions and attacks that Monsters have - that would be my preference.
That data could be used directly in a highly automated system, but it could also just be used to display information about the trait. Even if it's not being used to automate attacks or whatever, if it's not there, there's nothing in the data representing the Breath Weapon part of the trait.
What does this do?
groupof features withparent.It's type is APIReference, and it points to the parent feature.choiceattribute, it has been moved within the newly created attributefeature_specific. It has also been renamed according to what the choice entails. Also, any choices that used features when they could have used other data structures (ex: proficiency for expertise instead of a feature), have had their excess features removed and their choice data type changed.draconic-ancestryracial trait, and removes thetrait_optionsattribute from the dragonborn rave.How was it tested?
Wasn't yet. Intend to run tests on locally run db.
Is there a Github issue this is resolving?
No.
Did you update the docs in the API? Please link an associated PR if applicable.
Not yet, I intend to.
Here's a fun image for your troubles