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

Start refactoring character sheet get data #15476

Open
wants to merge 1 commit into
base: master
Choose a base branch
from

Conversation

CarlosFdez
Copy link
Collaborator

This should make it easier to follow along and also create room to start adding new things.

Some more refactoring needs to be done, but it'll require more intrusive updates to other creature sheets.

@stwlam
Copy link
Collaborator

stwlam commented Jul 19, 2024

This looks like it's all thrown in the returned object, making it harder to read.

@CarlosFdez
Copy link
Collaborator Author

CarlosFdez commented Jul 19, 2024

Its generally easier to read what data we're returning and its structure when its part of a final return object, even more so after some of the more complicated sub-sections have been moved to private helpers. Significantly better than getting the parent data and casting it to the new return type, and then bolting things on there after the fact and in helper methods and hoping we didn't miss anything. There's also type safety concerns as well.

Unfortunately, this isn't complete. There's still that weirdness with how saves and proficiencies are set up. Skills and saves can be lifted up to the creature level, and we should choose between whether to use short (character) or labelShort (NPC) for shortform save labels. We also need to umbrella some of the data, but what umbrellas to use I haven't decided on yet.

@CarlosFdez
Copy link
Collaborator Author

CarlosFdez commented Jul 19, 2024

....in short, please read character sheet getData() again (without this change), and then party sheet or deity sheet getData(), and let me know which is actually easier to follow along and get a sense of the structure. I was making an initial step to make the former like the latter, but we can instead make everything like character instead if we prefer it that way.

@stwlam
Copy link
Collaborator

stwlam commented Jul 19, 2024

While not asserting is nice, giving up separation of individual components of the sheet data isn't. There's a mix of both here, making for a net loss in readability.

@CarlosFdez
Copy link
Collaborator Author

CarlosFdez commented Jul 19, 2024

I find that claim pretty tough. I could not follow along at all when I tried to add has-stowing to it. There's no proper trail to follow, and having no type safety was scary on top of that. While there's more work to be done on that front, it definitely went from a 5 to a 20 when the scale is 100, as I can now scroll to the end of getData and usually search where data gets sets.

I could further the separations with some more info on where its problematic, perhaps with a more thorough refactor than I was planning for first pass. But let me know if you'd prefer the character sheet's paradigm, because I can convert all newer sheets to that style instead.

@CarlosFdez
Copy link
Collaborator Author

CarlosFdez commented Jul 19, 2024

Anyways, I'm going to need something actionable. I can either take this further, or consider the merge workflow we're doing elsewhere a lost cause and revert to this style. #15482 is a trial run converting other things to be like character sheet instead (but it still has the prepareX functions. Character sheet does not split its processing). if the trial run is preferred I can convert everything instead.

EDIT: I don't understand what you mean by lack of separating though. Its all a single object either way?

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants