-
-
Notifications
You must be signed in to change notification settings - Fork 126
Adding VolatileStatus #556
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
Conversation
…atileStatuses, whether effects come from abilities or moves, edited volatile_status in Move to return an Effect, added a way to convert strings in static data to Effects, and added testing
Codecov ReportAll modified and coverable lines are covered by tests ✅
Additional details and impacted files@@ Coverage Diff @@
## master #556 +/- ##
==========================================
+ Coverage 83.38% 84.48% +1.10%
==========================================
Files 39 42 +3
Lines 3918 4177 +259
==========================================
+ Hits 3267 3529 +262
+ Misses 651 648 -3 |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This looks good, but I'm concerned about maintaining all categories of effects up to date.
Can you add:
- a check that all effects is from either an ability, item or move, and only one? (if this is true?)
- all volatile effects appear in at least one move?
- all move's volatile effects are a) an effect and b) volatile?
- any other automated check you can think of
try: | ||
return _FROM_DATA[message] | ||
except KeyError: | ||
logging.getLogger("poke-env").warning( |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Can you add a test for this path? Eg. calling from_data
on a non-existing effect
…ect, and that Effect doesn't record any superfluous VSs. Also fixed a couple of errors and cleared bad grammar in a comment
It was a good call to add these. I think what you came up with was pretty comprehensive and I definitely caught some stuff. This test setup should ensure that if new volatile_statuses are added, Effects will get flagged (as long as we update the move_generator to the most recent gen). Because you manually updated Effects with all the other showdown effects, I'm not sure how to ensure they get added; however, if they are added, the person who added them will have to all ensure that the class variables also get updated too |
That sounds good - thanks @caymansimpson ! |
Added all VolatileStatus to Effects, added tracking in Effect for VolatileStatuses, whether effects come from abilities or moves, edited volatile_status in Move to return an Effect, added a way to convert strings in static data to Effects, and added testing
Idea here is that we are able to allow AI to better understand effects, and link them to moves/abilities