Create return matcher for extensible return widget creation

[gselzer]

This PR introduces a class ReturnMatcher, an analogue of TypeMatcher for widgets displaying function outputs.

As of now, there are only two ReturnMatchers written. One supports "simple" types via widgets.LineEdit. The other supports "tabular" types via widgets.Table. There is nothing preventing us from writing more of these to support other types.

The default return widget is a widgets.LineEdit, chosen to best align with previous behavior.

[tlambert03]

generally looks good! I think, if possible, I'd prefer not to have an entirely different module for return annotations. If you can point to the specific places that made you feel like you needed a fully different get_widget_class function and new module, I might be able to suggest solutions that would prevent all the code duplication

[gselzer]

If you can point to the specific places that made you feel like you needed a fully different get_widget_class function and new module, I might be able to suggest solutions that would prevent all the code duplication

I don't know that there were any, that might just be my java background showing 😅

If we propagate the is_input parameter into type_map.py, we should be able to merge the two classes. I'll do that quick.

Do you want to leave the TypeMatcher class the way it is, or should we call it ParamMatcher to align with the ReturnMatcher class?

[tlambert03]

If we propagate the is_input parameter into type_map.py, we should be able to merge the two classes. I'll do that quick.

even more, can you think of specific scenarios where we want to map a given type to a different widget on the output than we would on the input? Most of the type->widget mappings kind of make sense for the value type (independent of whether it's an input or an output). The one exception might be that outputs should be enabled=False

in other words, I'm saying why can't we just add a new type for table here and be done with it?

[gselzer]

can you think of specific scenarios where we want to map a given type to a different widget on the output than we would on the input?

Sure, I mean one I think makes the least sense would be a bool, where you'd want a check box if input, but that probably doesn't make a ton of sense for the output.

Similarly, you might want a slider widget for a numerical input, but I don't think you'd want that for the output

[tlambert03]

Sure, I mean one I think makes the least sense would be a bool, where you'd want a check box if input, but that probably doesn't make a ton of sense for the output.

doesn't it though? If someone wrote:

@magicgui(result_widget=True)
def some_widget(...) -> bool:
    ...

wouldn't a checkbox that ticks and unticks itself as the result changed from True to false be as reasonable of a default as any? I see that you're using LineEdit which is also fine... but I think we need to have a relatively high bar here to justify a totally different type matcher.

I do think that anything that requires further input (like the filedialog for pathlib.Path) is a good example of something that doesn't make sense as an output.

Lemme ponder this one a bit :)

[gselzer]

@tlambert03 I condensed the two type matchers into one module, but there are still two matchers. I did actually find a difference between the two that wasn't just a difference in return widget; I get test failures in other tests if I use this block in return widget matching.

[tlambert03]

this is looking good @gselzer! Thank you also for the lovely tests.

I've made few minor comments about naming and API. But other than that I think it's very close!

[tlambert03]

this is technically a breaking API change, since this is a public function and this changes the order of positional args. (not to mention the fact that the current default of is_input=None will change this function to return widgets by default). Can we put this after options, change the name to is_result (to match the current wording used elsewhere of result_widget, and use a default value of is_result=False?

[gselzer]

I made the suggested change and rebased it into the commit that added this so that we wouldn't have any commits where is_input existed. Please let me know what you think.

[tlambert03]

see other comment, move after options and change to is_result=False

[gselzer]

I made the suggested change and rebased it into the commit that added this so that we wouldn't have any commits where is_input existed. Please let me know what you think.

[tlambert03]

also, looks like there's one last failing test: https://github.com/napari/magicgui/runs/4913849327?check_suite_focus=true#step:6:296

[gselzer]

also, looks like there's one last failing test: https://github.com/napari/magicgui/runs/4913849327?check_suite_focus=true#step:6:296

@tlambert03 I don't understand that test failure. Why is a function in test_magicgui.py even being called?

[tlambert03]

also, looks like there's one last failing test: https://github.com/napari/magicgui/runs/4913849327?check_suite_focus=true#step:6:296

@tlambert03 I don't understand that test failure. Why is a function in test_magicgui.py even being called?

ah, because of a poorly designed test causing side effects over there :)

in line ~446 of test_magicgui.py, add this try/finally, and pop the registered types.

    register_type(int, return_callback=check_value)
    register_type(Base, return_callback=check_value)

    try:
        @magicgui
        def func(a=1) -> int:
            return a

        func()
        with pytest.raises(AssertionError):
            func(3)

        @magicgui
        def func2(a=1) -> Sub:
            return a

        func2()
    finally:
        from magicgui.type_map import _RETURN_CALLBACKS
        _RETURN_CALLBACKS.pop(int)
        _RETURN_CALLBACKS.pop(Base)

(if this happens again, I can deal with this on a more global level)

[gselzer]

Gotcha, that seems to have fixed it on my side @tlambert03. Let me know what you think

[codecov]

Codecov Report

Merging #355 (0352021) into main (65182ec) will decrease coverage by 0.14%.
The diff coverage is 85.29%.

@@            Coverage Diff             @@
##             main     #355      +/-   ##
==========================================
- Coverage   90.35%   90.21%   -0.15%     
==========================================
  Files          29       29              
  Lines        3349     3402      +53     
==========================================
+ Hits         3026     3069      +43     
- Misses        323      333      +10     

Impacted Files	Coverage Δ
magicgui/type_map.py	92.20% <79.16%> (-3.71%)	⬇️
magicgui/types.py	100.00% <100.00%> (ø)
magicgui/widgets/_bases/create_widget.py	97.29% <100.00%> (+0.07%)	⬆️
magicgui/widgets/_function_gui.py	94.04% <100.00%> (+0.02%)	⬆️
magicgui/widgets/_table.py	96.80% <100.00%> (+0.02%)	⬆️

---

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 65182ec...0352021. Read the comment docs.

[tlambert03]

Thanks a lot for this @gselzer! very handy addition!