canismarko / dungeon-sheets

A tool to create character sheets and GM session notes for Dungeons and Dragons fifth edition (D&D 5e).
https://dungeon-sheets.readthedocs.io/en/latest/
GNU General Public License v3.0
161 stars 66 forks source link

Featurestats #159

Closed PJBrs closed 2 weeks ago

PJBrs commented 7 months ago

This PR aims to clean up some code in feature application, adds more racial spells automatically, adds class-based darkvision, and adds some feat bonuses to initiative.

For review, I'd like to point out two things:

  1. I removed all spell stuff from race.py
  2. Some races include first level spells at second level, which I implemented as follows:
+    class HellishRebukeAtLvl2(spells.HellishRebuke):
+        level = 2
+        __doc__ = spells.HellishRebuke.__doc__.replace('2nd', '3rd').replace('1st', '2nd')
+        name = "Hellish Rebuke (Racial Feature)"
+
+    _spells = {1: [spells.Thaumaturgy,], 3: [HellishRebukeAtLvl2,]}
+    spells_known = []
+    spells_prepared = []
PJBrs commented 6 months ago

I noticed that this PR was sitting here for a while already, which made me have another look at these patches. On second thought, I think I was wrong in the way I tried to implement, e.g., Hellish Rebuke at level 2, in the sense that that wasn't according to the rules. And on closer inspection so many of these racial feature spells follow specific rules, that perhaps it is easier to just add them and let the player closely read their feats. If you want, I can squash the first and fourth patches.

canismarko commented 6 months ago

Yeah sorry, a lot of these PRs accomplish more than one thing which makes it time-consuming to review and I've been very busy lately for reasons that aren't worth getting into here. I really do appreciate you putting in the considerable time to contribute these improvements.

1e7d69d19b8557ccc6dc2d3c68c96ea441046ded and e01e9e9269491dfc056e64524d70d7e57d8c07fc are good.

You are proposing undoing cc7be9d139fce8bb9d51e68b5e44e1fb4602bf2e and 440236b29dbaa1d1b649643c7e709ad0180a3bad, right?

I'm fine with that. If you want to push commits that do that, I will merge this PR as is.

PJBrs commented 6 months ago

You are proposing undoing cc7be9d and 440236b, right?

No, not quite, 440236b is a partial undo of cc7be9d. But I can see that perhaps in these two patches I wanted to accomplish a little bit to much. I'll change it a bit and explain better....

PJBrs commented 6 months ago

So, I split the most complex patch in this series. The two patches that you wanted to merge in any case are: https://github.com/canismarko/dungeon-sheets/pull/159/commits/7727c1385c995e464ad07a142bd0bf0a65ffedf1 and https://github.com/canismarko/dungeon-sheets/pull/159/commits/e005acdf9da635969b0ec2b4bd38ec572499faf3

The rest of this PR I now split in three different patches. The first one (https://github.com/canismarko/dungeon-sheets/pull/159/commits/443a5afa359985e3e4d0a705bf97c77d805594be) adds more racial spells.

The final two patches together first (https://github.com/canismarko/dungeon-sheets/pull/159/commits/9ec10b8aa4efb4ed7a531bdc1de14f1b1921256d) move any remaining spells from race.py to features/race,py, and then (https://github.com/canismarko/dungeon-sheets/pull/159/commits/dab29c73ffbe7cc81cbf9167dbfc4ae8fca06667) remove all spell-related code from race.py.

The only change with the previous push I did for this PR is that I removed all cosmetic changes to one big spelling fixes patch that I've saved locally. Functionally, there's no changes.

Let me know if this works better for you. If not, just tell me which patches to remove!

Also, thanks for taking the time to review this, and the best of luck dealing with those busy times!

PJBrs commented 5 months ago

@canismarko Anything else I can do for this PR?