Skip to content
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

Fix bug in creation of RangeEdit using create_widget #396

Merged
merged 3 commits into from
Apr 18, 2022

Conversation

Czaki
Copy link
Contributor

@Czaki Czaki commented Apr 6, 2022

When run this code

from magicgui.widgets import create_widget

w = create_widget(annotation=range)

it ends with error:

Traceback (most recent call last):
  File "/home/czaki/Projekty/neutrofile/Tutorial/sample_magicgui.py", line 3, in <module>
    w = create_widget(annotation=range)
  File "/home/czaki/.pyenv/versions/3.8.3/envs/neutrofile/lib/python3.8/site-packages/magicgui/widgets/_bases/create_widget.py", line 96, in create_widget
    widget = wdg_class(**kwargs)
  File "/home/czaki/.pyenv/versions/3.8.3/envs/neutrofile/lib/python3.8/site-packages/magicgui/widgets/_concrete.py", line 558, in __init__
    raise TypeError(f"Invalid value type for {type(self)}: {type(value)}")
TypeError: Invalid value type for <class 'magicgui.widgets.RangeEdit'>: <class 'magicgui.widgets._bases.value_widget._Unset'>

When fixing this error by checking if value is None or UNSET then also happens problem with an unexpected nullable field.
The second error I fix by pop the nullable from kwargs. But I'm not sure if it is the correct solution.

@codecov
Copy link

codecov bot commented Apr 6, 2022

Codecov Report

Merging #396 (db95b7d) into main (56e419d) will decrease coverage by 0.02%.
The diff coverage is 100.00%.

@@            Coverage Diff             @@
##             main     #396      +/-   ##
==========================================
- Coverage   88.61%   88.59%   -0.03%     
==========================================
  Files          30       30              
  Lines        3796     3797       +1     
==========================================
  Hits         3364     3364              
- Misses        432      433       +1     
Impacted Files Coverage Δ
magicgui/widgets/_concrete.py 85.42% <100.00%> (+0.02%) ⬆️
magicgui/_type_wrapper.py 67.87% <0.00%> (-0.37%) ⬇️

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update 56e419d...db95b7d. Read the comment docs.

@tlambert03
Copy link
Member

thanks @Czaki! These changes make sense / work for me. more broadly, I think the issue here stems from having these "container-based" classes that more or less implement the ValueWidget protocol, but don't directly inherit from valuewidget. (see https://github.com/napari/magicgui/issues/372).

This change is totally fine with me as a first step... maybe in the future, I can get around to making them actually be ValueWidgets (in which case nullable should work again, and allow things like create_widget(annotation=Optional[range])

@Czaki
Copy link
Contributor Author

Czaki commented Apr 7, 2022

ValueWidget

it will be also nice if create_widget return ValueWidget, not Widget (type annotation, documentation etc)

@tlambert03
Copy link
Member

This came up before in some issue, can't find it atm, since not all widgets are necessarily value widgets, I'm not sure that's the proper annotation?

@Czaki
Copy link
Contributor Author

Czaki commented Apr 7, 2022

Hm. In general I know because Container is also widget. But what input to create_widget could create non-value widget?

Or you think about user registered widget?

@tlambert03
Copy link
Member

But what input to create_widget could create non-value widget?

Yeah, thats a good question :)

I can't immediately think of one. I'm open to it. I remember trying to change it one point and then realizing that changing that annotation requires a lot of other type changes elsewhere, but it might be worth it. Feel free to dig a bit deeper if you're interested

@Czaki
Copy link
Contributor Author

Czaki commented Apr 7, 2022

If I should fix somehow failing test? It looks non-related.

@tlambert03 tlambert03 merged commit 7013061 into pyapp-kit:main Apr 18, 2022
@Czaki Czaki deleted the bugfix/range_unset branch April 19, 2022 09:19
@tlambert03 tlambert03 added the bug Something isn't working label Jun 13, 2022
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
bug Something isn't working
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants