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

Add Spellbook Functionality #23

Open
wants to merge 14 commits into
base: main
Choose a base branch
from
Open

Conversation

stevebelew
Copy link

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.

  • Added Spellbook class in a new file spellbook.py.
  • Implemented methods add_spell, remove_spell, validate_spell, and others to manage spells in a spellbook.
  • Added test_spellbook.py with tests for various scenarios, ensuring the correct behavior of the spellbook methods.
  • All test cases pass, verifying the implementation aligns with the expected behavior.

Your review and feedback are highly appreciated.

@tassaron tassaron linked an issue Oct 5, 2023 that may be closed by this pull request
2 tasks
@tassaron
Copy link
Owner

tassaron commented Oct 5, 2023

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 :)

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.

Can't put spells in spellbooks
2 participants