refactor(models): 🎨 abstract model decorators into single utility decorator#806
refactor(models): 🎨 abstract model decorators into single utility decorator#806faewd wants to merge 8 commits into5e-bits:mainfrom
Conversation
|
@faewd Is this ready for review? |
Oh yep, I reckon so! |
bagelbits
left a comment
bagelbits
left a comment
There was a problem hiding this comment.
There might be some mismatch across the classes of what is and isn't optional? But I'm not sure how important it is.
Otherwise, I think it's a lot cleaner.
src/models/2014/trait.ts
Outdated
| @prop({ type: () => [APIReference], required: true }) | ||
| @field(() => T.RefList(Subrace), { | ||
| description: 'Subraces that possess this trait.', | ||
| optional: true |
src/models/2014/trait.ts
Outdated
|
|
||
| @Field(() => [Race], { nullable: true, description: 'Races that possess this trait.' }) | ||
| @prop({ type: () => [APIReference], required: true }) | ||
| @field(() => T.RefList(Race), { description: 'Races that possess this trait.', optional: true }) |
src/models/2014/trait.ts
Outdated
| export class ActionDamage { | ||
| @Field(() => DamageType, { nullable: true, description: 'The type of damage dealt.' }) | ||
| @prop({ type: () => APIReference }) | ||
| @field(() => T.Ref(DamageType), { description: 'The type of damage dealt.', optional: true }) |
There was a problem hiding this comment.
I must have based these on the nullable: true in the original Field decorator, but yeah I think you're right that none of these 3 should be optional. Will fix tomorrow :)
There was a problem hiding this comment.
Would you like me to review and flag other files as well? I only looked at one and stopped.
There was a problem hiding this comment.
Some additional thoughts:
In the TS model here, subraces and races are both optional (with the ? suffix), which makes sense as a trait either belongs to a race or a subrace. But in the actual data, we're always populating both, and just leaving the subraces array empty if it belongs to a race, and vice versa.
Interestingly I'm not sure any of the traits belong to more than one race or subrace, so I'm not sure what the original intention was behind making these arrays rather than nullable fields.
Either way, updating the model and the data would be out of scope of this PR, so for the time being, so that the models reflect the data, I suppose these fields shouldn't be optional, and both the decorator and the underlying class property need updating.
There was a problem hiding this comment.
Would you like me to review and flag other files as well? I only looked at one and stopped.
Oh yes please! I think another pair of eyes to spot any other discrepencies that I've failed to correct, or mistakes that I've made, wouldn't go amiss!
bagelbits
left a comment
bagelbits
left a comment
There was a problem hiding this comment.
Thank you for doing this!
| public action_options?: Choice | ||
| } | ||
|
|
||
| type ArmorClass = |
There was a problem hiding this comment.
Can you update this type, export it, and use it in the factory as well?
2 participants
What does this do?
Uses a custom
fielddecorator to reduce complexity & duplication in Type-GraphQL'sFieldand Typegoose'spropdecorators.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>
Was any impacted documentation updated to reflect this change?
<It's not clear if I don't update this text with relevant info>
Here's a fun image for your troubles
<Add a fun image here>