Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

[#1405] Modify trait utilities to support prefixed trait keys & add SelectChoices #2499

Merged
merged 4 commits into from
Oct 25, 2023

Conversation

arbron
Copy link
Collaborator

@arbron arbron commented Oct 19, 2023

  • Modifies a number of utility methods in trait utilities to support prefixed trait keys (tool:art:potter instead of just potter)
  • Introduces the SelectChoices type which offers a number of methods for filtering, sorting, cloning, etc. nested sets of options like those represented by tool categories

@arbron arbron added this to the D&D5E 2.4.0 milestone Oct 19, 2023
@arbron arbron requested a review from Fyorl October 19, 2023 20:16
@arbron arbron self-assigned this Oct 19, 2023
@Fyorl Fyorl added the breaking Breaking changes label Oct 20, 2023
Copy link
Contributor

@Fyorl Fyorl left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I can't help but feel like we perhaps wouldn't need to do quite so much work in Trait if we had more consistent and predictable structuring in our CONFIG as well as for where traits are stored on the actor. I'm not sure there's enough of a tangible benefit to changing it though at this stage given how much churn and breakage it would cause.

It seems that we only really ever have one level of children in our current trait configuration. I wonder, then, if having a fully-recursive tree structure and associated operations is necessary, and if there are maybe some simplifications that can be won from recognising this property. It might be that the general tree solution is the simplest though.

In general though I think it's fine that this complexity is tucked away in these utilities. It's an acceptable place to be currently.

If I could ask though for some small examples as to some of the inputs and outputs for the SelectChoices class, particularly in regard to how the wildcards work. That would help me a lot with contextualising some of this work. Since it's all sets of strings I'm hoping they won't be too arduous to produce. We could also include them as @example annotations.

Some examples for some of the trait methods would also help me, like you have in choiceLabel, they were very useful.

module/documents/actor/select-choices.mjs Outdated Show resolved Hide resolved
module/documents/actor/select-choices.mjs Outdated Show resolved Hide resolved
module/documents/actor/select-choices.mjs Outdated Show resolved Hide resolved
module/documents/actor/select-choices.mjs Outdated Show resolved Hide resolved
module/documents/actor/select-choices.mjs Outdated Show resolved Hide resolved
module/documents/actor/trait.mjs Outdated Show resolved Hide resolved
module/documents/actor/trait.mjs Outdated Show resolved Hide resolved
module/documents/actor/trait.mjs Outdated Show resolved Hide resolved
module/documents/actor/trait.mjs Outdated Show resolved Hide resolved
module/documents/actor/trait.mjs Outdated Show resolved Hide resolved
@arbron arbron force-pushed the advancement/trait-utils branch 3 times, most recently from 4de6617 to 6941530 Compare October 20, 2023 21:05
@arbron arbron requested a review from Fyorl October 20, 2023 21:06
@arbron
Copy link
Collaborator Author

arbron commented Oct 20, 2023

I can't help but feel like we perhaps wouldn't need to do quite so much work in Trait if we had more consistent and predictable structuring in our CONFIG as well as for where traits are stored on the actor. I'm not sure there's enough of a tangible benefit to changing it though at this stage given how much churn and breakage it would cause.

While I think it would be okay to switch things on the actor side, any major change of the config would be a breaking change for any 3rd party content that adds new types.

It seems that we only really ever have one level of children in our current trait configuration. I wonder, then, if having a fully-recursive tree structure and associated operations is necessary, and if there are maybe some simplifications that can be won from recognising this property. It might be that the general tree solution is the simplest though.

We do have the chance of multiple levels when more than one trait type is represented, which mainly comes into play with expertise (e.g. you get a choice between gaining expertise in a any skill or thieves tools).

If I could ask though for some small examples as to some of the inputs and outputs for the SelectChoices class, particularly in regard to how the wildcards work. That would help me a lot with contextualising some of this work. Since it's all sets of strings I'm hoping they won't be too arduous to produce. We could also include them as @example annotations.

Added some examples for filter and exclude.

Some examples for some of the trait methods would also help me, like you have in choiceLabel, they were very useful.

Add a bunch and moved the examples into the JSDocs.

Copy link
Contributor

@Fyorl Fyorl left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Looks pretty good to me, thank you. I've left some minor linting comments and some suggestions for consideration, that you can take or leave.

module/documents/actor/select-choices.mjs Outdated Show resolved Hide resolved
module/documents/actor/select-choices.mjs Outdated Show resolved Hide resolved
module/documents/actor/select-choices.mjs Show resolved Hide resolved
module/documents/actor/select-choices.mjs Outdated Show resolved Hide resolved
module/documents/actor/trait.mjs Show resolved Hide resolved
module/documents/actor/trait.mjs Show resolved Hide resolved
module/documents/actor/trait.mjs Outdated Show resolved Hide resolved
module/documents/actor/trait.mjs Outdated Show resolved Hide resolved
module/documents/actor/select-choices.mjs Outdated Show resolved Hide resolved
@arbron arbron requested a review from Fyorl October 23, 2023 20:38
Copy link
Contributor

@Fyorl Fyorl left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Just one comment above about adding documentation then feel free to merge.

@arbron arbron merged commit 2c59ee5 into foundryvtt:2.4.x Oct 25, 2023
@arbron arbron deleted the advancement/trait-utils branch October 25, 2023 16:44
@arbron arbron mentioned this pull request Oct 29, 2023
7 tasks
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

2 participants