-
Notifications
You must be signed in to change notification settings - Fork 52
feat: Add mechanism to check basic subclassing of generics #658
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
Codecov ReportAll modified and coverable lines are covered by tests ✅
Additional details and impacted files@@ Coverage Diff @@
## main #658 +/- ##
==========================================
+ Coverage 89.12% 89.18% +0.05%
==========================================
Files 39 39
Lines 4718 4742 +24
==========================================
+ Hits 4205 4229 +24
Misses 513 513 ☔ View full report in Codecov by Sentry. |
seems reasonable to me, thanks @Czaki. @hanjinliu, it appears that this PR might break the magic-class tests. Could you look at the error and let me know if you think due to over-assumptions made on the magic-class side (which we should just ignore here), or whether it's indicative of something that could be a more general breaking change? |
src/magicgui/type_map/_type_map.py
Outdated
Sequence[pathlib.Path]: widgets.FileEdit, | ||
Sequence: widgets.ListEdit, |
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.
maybe even more generic and use Iterable
instead of Sequence
?
It was a bug with handling sequence of Paths. |
@Czaki , thank you for fixing this. At the time I install this branch it was already fixed 😅 from magicgui import magicgui
@magicgui(x={"bind": [1]})
def func(x: list[int]):
print(x)
func.show(True)
I hit same error when I use |
I see this error also on main. So it looks like some previous PR introduced it. I do not have experience with bind. Maybe @tlambert03 will know. |
yeah lets copy that new a new issue. Thanks for digging into the sequence of paths thing @Czaki, I'll give this a final review later today then merge and can cut a new release if you'd like |
the quick answer is that |
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
If you could inform on pointed zulip topic, when you release a new version? |
This PR is addressing problem pointed by @psobolewskiPhD on napari zulip https://napari.zulipchat.com/#narrow/stream/309872-plugins/topic/List.20of.20LayerDataTuples.20yielded.20by.20a.20plugin.20not.20working.3F/near/454038830
The core of the problem is that if one registers
return
callback forlist[int]
it will not work with functions annotated withtyping.List[int]
and vice versa.This Pr is enhancing
safe_issubclass
to perform inheritance checking on generic as well. My goal is to allow useSequence[LayerDataTuple]
when registering type for magicgui widget and allow a user to specifylist[LayerDataTuple]
,typing.List[LayerDataTuple]
,tuple[LayerDataTuple, ...]
etc.I'm not sure if the impact of this solution on performance is significant.
What is not suported.
Tuple[something]
.[]