-
Notifications
You must be signed in to change notification settings - Fork 10
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
Make required_keys
always a set
#383
Conversation
Codecov ReportAttention: Patch coverage is
Additional details and impacted files@@ Coverage Diff @@
## dev_master #383 +/- ##
==============================================
+ Coverage 74.92% 74.94% +0.02%
==============================================
Files 56 56
Lines 7768 7772 +4
==============================================
+ Hits 5820 5825 +5
+ Misses 1948 1947 -1 ☔ View full report in Codecov by Sentry. |
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.
Good, a bit more clear.
I think there should be a required_keys = set()
in the top level Effect
class, but you did not change that file so I can't make a suggestion.
Yeah, I'm planning to do that eventually, when moving the required keys check there (well, then I'll have to do it anyway, or get an AttributeError for some subclasses). But the corresponding utils function is used not only in the effects, but also in an optics submodule somewhere, so probably end up pointing back there from the |
Ok I now included that already anyway, cause why not. |
Harmonize the
required_keys
of all effects to be a class attribute (as it will never change across instances) and aset
(as that makes it simpler and more efficient to compare against the supplied keys in a dict using set methods). In the future, I'll probably move this check to theEffect
base class and run it as part of thesuper().__init__()
call in each effect (i.e.required_keys
is an emptyset
in the base class, and overridden in the effect subclasses that need it).Note: Yes, this is more dev wåñk. I did these changes a while ago, but never pushed and PR'd...