tassaron / dnd-character

library for making Dungeons & Dragons 5e characters as serializable data
https://pypi.org/project/dnd-character/
Eclipse Public License 2.0
45 stars 17 forks source link

Add Spellbook Functionality #23

Open stevebelew opened 1 year ago

stevebelew commented 1 year ago

Hi,

This pull request introduces the Spellbook class along with methods for adding, removing, and validating spells based on D&D 5e rules. Additionally, it includes tests to ensure the correct functionality of these methods.

Your review and feedback are highly appreciated.

tassaron commented 1 year ago

Thanks!

I like how you added the scribe cost in gold pieces... I forgot about that. However, this cost doesn't apply when leveling up -- it's only for copying spells out of another spellbook, even though it makes no logical sense. That's game design trumping logic, I suppose. Maybe there should be a separate copy_spell function?

Your fetch_wizard_spells_from_json function is actually duplicating work done elsewhere. The spellcasting module has functions already for getting spell lists.

from dnd_character.spellcasting import spells_for_class_level
wizard_spells_by_level = {i: spells_for_class_level("wizard", i) for i in range(9)}

(That is a "dict comprehension")

The only difference between this code and your existing code is: that this wizard_spells_by_level has indexes instead of names (e.g., "acid-arrow" instead of "Acid Arrow"). You can then get the spell data from SPELLS['acid-arrow'] (also in spellcasting module)

I would recommend using these spell objects from SPELLS in your test function too, rather than using mocks. Mocks are typically for expensive resources such as those returned from the internet. (An example would be, mocking S3 storage to pretend that a file failed to upload, then testing how the program reacts to that failure which wouldn't otherwise have occurred.)

Rename run_tests to test_spellbook, and remove the final line where you call that function -- pytest (the test runner) automatically runs any function that starts with test_

Otherwise I don't see other significant issues. I'll be happy to help more in the future :)