Skip to content

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

Merged
merged 7 commits into from
Jul 29, 2024

Conversation

Czaki
Copy link
Contributor

@Czaki Czaki commented Jul 27, 2024

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 for list[int] it will not work with functions annotated with typing.List[int] and vice versa.

This Pr is enhancing safe_issubclass to perform inheritance checking on generic as well. My goal is to allow use Sequence[LayerDataTuple] when registering type for magicgui widget and allow a user to specify list[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.

  1. NamedTuple, when registered type, is plain Tuple[something].
  2. Unions in []

Copy link

codecov bot commented Jul 27, 2024

Codecov Report

All modified and coverable lines are covered by tests ✅

Project coverage is 89.18%. Comparing base (383776c) to head (1479122).

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.
📢 Have feedback on the report? Share it here.

@tlambert03
Copy link
Member

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?

Comment on lines 74 to 75
Sequence[pathlib.Path]: widgets.FileEdit,
Sequence: widgets.ListEdit,
Copy link
Contributor Author

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?

@Czaki
Copy link
Contributor Author

Czaki commented Jul 29, 2024

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?

It was a bug with handling sequence of Paths.

@hanjinliu
Copy link
Collaborator

@Czaki , thank you for fixing this. At the time I install this branch it was already fixed 😅
I accidentally found another bug, which I did not see before.

from magicgui import magicgui

@magicgui(x={"bind": [1]})
def func(x: list[int]):
    print(x)

func.show(True)
TypeError: ListEdit got an unexpected keyword argument: bind

I hit same error when I use tuple[int, str]. Do you have any idea why this is happening?

@Czaki
Copy link
Contributor Author

Czaki commented Jul 29, 2024

@Czaki , thank you for fixing this. At the time I install this branch it was already fixed 😅 I accidentally found another bug, which I did not see before.

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.

@tlambert03
Copy link
Member

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

@tlambert03
Copy link
Member

I hit same error when I use tuple[int, str]. Do you have any idea why this is happening?

the quick answer is that bind is a parameter in ValueWidget, but list and tuple are container widgets (and do not inherit ValueWidget).

Copy link
Member

@tlambert03 tlambert03 left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

LGTM

@tlambert03 tlambert03 merged commit a9ea08e into pyapp-kit:main Jul 29, 2024
36 checks passed
@Czaki Czaki deleted the better_generic branch July 29, 2024 14:05
@Czaki
Copy link
Contributor Author

Czaki commented Jul 29, 2024

If you could inform on pointed zulip topic, when you release a new version?

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
enhancement New feature or request
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants