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

Collapse stat and save #11

Closed
wants to merge 12 commits into from
Closed

Conversation

pmfirestone
Copy link

This collapses the types for the stat rolls and the save rolls into one another. This makes it easier to modify the logic and centralize it. This is mentioned in #6.

We're already using vite, so this is the appropriate way to do unit testing.
This was hard to get working, but might be interesting in the future.
Add three sucess tests, as an initial frame for testing.
Each player character shows a summary of their skills in the summarized
character sheet in the left bar.
This button does more harm than good until armor is implemented.
The logic of analyseStatRoll and analyseSaveRoll are identical. The logic is
somewhat convoluted and does not accurately implement the behavior specified
in the rule book. Before fixing it, take the opportunity to solve remove the
duplicated code to keep from having to fix it twice.
These are the four detailed roll examples I could find in the PSG. This shook
out an error in the logic.
These are a couple of changes missed in commit 2e8ac1b.
This collapses the types of stat and save rolls into one and simplifies the
hierarchy. It does not change the function of the rolls, but it does make them
easier to maintain.
@pmfirestone
Copy link
Author

The pull is rather nasty since I've had to corroborate several branches. I'd actually not accept this yet, since there's some extra changes in the middle there. Unfortunately, and this is my fault, I've hacked my code base to the point where it's hard to pull out the changes I need to. I'm sure there's a way to do it with git, or I could manually confect the changes I need, but right now this is what I can do.

@pmfirestone pmfirestone closed this Mar 6, 2024
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.

2 participants