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

Feature & Trait Choices #380

Merged
merged 26 commits into from
Jul 9, 2021
Merged

Feature & Trait Choices #380

merged 26 commits into from
Jul 9, 2021

Conversation

Wyzards
Copy link
Contributor

@Wyzards Wyzards commented Jul 8, 2021

What does this do?

  • Replaces Feature's group attribute with the parent attribute. Its type is APIReference, and it points to the parent feature.
  • Moves Feature's choice attribute into Feature's feature_specific attribute. Also renames it (either to subfeature_options, expertise_options, spell_options or trait_options for trait equivalents.
  • Changes choice type of rogue-choose-expertise and bard-choose-expertise to proficiency.
  • Removes unnecessary bard and rogue expertise features.
  • Adds a choice structure to the trait draconic_ancestry's feature specific. Also includes draconic_ancestry's damage type and breath weapon action under trait_specific
  • Removes "Choose: " from the name of Features which involve making a choice.
  • removes "choose-" and "-choose" from traits / features whos index includes it.

These changes were discussed in the API Discord between myself and @fergcb with some input from @bagelbits and @ogregoire in a different draft PR.

Discord discussion link: https://discord.com/channels/656547667601653787/656547667601653790/861810226541625395
Old draft PR link: #378

How was it tested?

Database was run locally on machine. Was built from remote git branch and ran tests according to api documentation (unit test, integration and controllers).

Is there a Github issue this is resolving?

No.

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

Yes, 5e-bits/5e-srd-api#226

Here's a fun image for your troubles

random photo - update me

@ogregoire
Copy link
Collaborator

This is good, and it was well needed. Your correction regarding the previous PR is accurate and makes the naming more consistent regarding previous changes (removing "Choose:" rather than adding it, even though we should go further on that topic). I no longer want this PR to be split into several different as it does two things but does them well.

Since the PR is in draft mode, I cannot approve it in the GitHub sense of it, but I'm approving it here in written.

Given the large amount of commits, I believe that this PR should be squash-merged. What's your opinion on this?

src/5e-SRD-Features.json Show resolved Hide resolved
src/5e-SRD-Features.json Outdated Show resolved Hide resolved
src/5e-SRD-Features.json Outdated Show resolved Hide resolved
@fergcb
Copy link
Member

fergcb commented Jul 8, 2021

Given the large amount of commits, I believe that this PR should be squash-merged. What's your opinion on this?

All multi-commit PRs in this repo are squashed by default, anyway, are they not?

@bagelbits
Copy link
Collaborator

All multi-commit PRs in this repo are squashed by default, anyway, are they not?

@fergcb @ogregoire That is correct. All PRs are Squash and Merge. It makes it easy to rollback/revert for both repos if something goes wrong.

@Wyzards Wyzards marked this pull request as ready for review July 9, 2021 01:15
@Wyzards Wyzards requested a review from ogregoire July 9, 2021 01:21
Copy link
Collaborator

@bagelbits bagelbits left a comment

Choose a reason for hiding this comment

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

Double approve since @ogregoire approved it too.

@bagelbits bagelbits merged commit d7c501b into 5e-bits:main Jul 9, 2021
@Wyzards Wyzards deleted the feature_choice branch July 18, 2021 03:30
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants