-
Notifications
You must be signed in to change notification settings - Fork 26
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
Switch abstract interfaces to protocols #120
base: main
Are you sure you want to change the base?
Switch abstract interfaces to protocols #120
Conversation
Codecov ReportAttention: Patch coverage is
Additional details and impacted files@@ Coverage Diff @@
## main #120 +/- ##
===========================================
+ Coverage 80.80% 95.02% +14.22%
===========================================
Files 37 39 +2
Lines 2849 3195 +346
===========================================
+ Hits 2302 3036 +734
+ Misses 547 159 -388 ☔ View full report in Codecov by Sentry. |
Passes regression tests: |
b4c6fcd
to
93bd7eb
Compare
2c1b09e
to
b85a10e
Compare
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.
LGTM aside from one docstring that looks like it was mistakenly copied from elsewhere
46e71f3
to
1349871
Compare
1349871
to
5cfeef2
Compare
5cfeef2
to
633dcea
Compare
This PR replaces the
AbstractDataModel
object with a python protocol as described in #118 (comment). This is a bit cleaner than the current implementation.This PR also implements aModelContainer
protocol which provides the same functionality of #118.Note that the only functional change is that
issubclass
will does not function quite correctly due to the missing__hash__
see python/mypy#3939 for a more detailed discussion of this. Since in general the idea of theAbstractDataModel
was to provide a way to check if some non-inheriting object instance followed the structure it described,issubclass
checks are unnecessary and likely only limited tostpipe
itself, which itself makes no such checks.