add list of list support to buttongroup widget

[jo47011]

You can now add list of buttons to the BouttonGroup widget. These will be displayed in a grouped style. Added a simple example to ui_showcase.py:

# A simple ButtonGroup with nested lists of ToggleButtons for foobar
index_page.add_item(ButtonGroup("State of the foobar (grouped)", [
    [ToggleButton("Foo").connect(foo),ToggleButton("Bar", color='red').connect(bar)],
    [ToggleButton("Foobar", color='black').connect(foobar)],
]))

[codecov]

Codecov Report

All modified and coverable lines are covered by tests ✅

Project coverage is 83.16%. Comparing base (905aed0) to head (f017ccd).
Report is 10 commits behind head on main.

Additional details and impacted files

@@            Coverage Diff             @@
##             main      #90      +/-   ##
==========================================
+ Coverage   82.75%   83.16%   +0.41%     
==========================================
  Files          42       42              
  Lines        5608     5614       +6     
  Branches      865      761     -104     
==========================================
+ Hits         4641     4669      +28     
  Misses        743      743              
+ Partials      224      202      -22     

☔ View full report in Codecov by Sentry.
📢 Have feedback on the report? Share it here.

[jo47011]

No idea why mypy wants this now here and not at other places, e.g. ui_showcase.py. Maybe you have an idea how to solve it without the cast. Looks kind of ugly.

[mhthies]

MyPy tries to guess the type of that list here, based on the first common ancestor class of the two buttons it finds. Unfortunately it finds the common ancestor WebDisplayDatapoint[bool] instead of AbstractButton. So it complains that we are passing a list of ``WebDisplayDatapoint[bool]s instead of a list of AbstractButtons`. I assume that, without the new `Union` in the parameter type hint, MyPy was smart enough to find that `List[AbstractButton]`, as expected by the parameter, is also a valid type of that list, so it used that instead of trying to guess the type.

I think, the "correct" solution would be to explicitly type-hint the list:

buttons: List[AbstractButton] = [
    ToggleButton(icon('volume mute')).connect(mute),
    DisplayButton(label=icon('power off')).connect(active),
]
page.add_item(ButtonGroup("", buttons))

However, that also looks kind of ugly, especially in the example code. So maybe, we go for a simple type-ignore with a comment, like # type: ignore # MyPy has problems with guessing the correct type of the list of buttons.
Maybe, MyPy will learn this type inference eventually and we can remove the comment. (I will check that once in a while by running MyPy with --warn-unused-ignores locally.)

[mhthies]

The remaining MyPy issues are unrelated to this PR and should exist on the main branch, as well. I assume that they are caused by a MyPy update, including new typing semantics or checks. (The CI pipeline always uses the latest MyPy version.)

[mhthies]

Thank you very much for diving into this and contributing the patch! :)

I'm overall really happy with your patch (and that's saying something, as I'm known for my profound code reviews at work). ;)

Maybe you can add a unit test for the new usage of the ButtonGroup? It should create a ButtonGroup with multiple groups and check that all buttons are present. Maybe also check that the buttons are correctly grouped, but that's a bonus. I will not strictly expect the test for merging the PR, but it would be nice.

[mhthies]

I like the use of ui horizontal list here. Sounds like the intended solution by SemanticUI. :)

[mhthies]

MyPy tries to guess the type of that list here, based on the first common ancestor class of the two buttons it finds. Unfortunately it finds the common ancestor WebDisplayDatapoint[bool] instead of AbstractButton. So it complains that we are passing a list of ``WebDisplayDatapoint[bool]s instead of a list of AbstractButtons`. I assume that, without the new `Union` in the parameter type hint, MyPy was smart enough to find that `List[AbstractButton]`, as expected by the parameter, is also a valid type of that list, so it used that instead of trying to guess the type.

I think, the "correct" solution would be to explicitly type-hint the list:

buttons: List[AbstractButton] = [
    ToggleButton(icon('volume mute')).connect(mute),
    DisplayButton(label=icon('power off')).connect(active),
]
page.add_item(ButtonGroup("", buttons))

However, that also looks kind of ugly, especially in the example code. So maybe, we go for a simple type-ignore with a comment, like # type: ignore # MyPy has problems with guessing the correct type of the list of buttons.
Maybe, MyPy will learn this type inference eventually and we can remove the comment. (I will check that once in a while by running MyPy with --warn-unused-ignores locally.)

[mhthies]

One afterthought: Can you please add a more detailed description on how to use the parameter, i.e. what effect the single list of button descriptors and the list of lists of button descriptors will have? You can either write it in the parameter description or as a separate paragraph above.

[jo47011]

Ready to go. Let me know whether you have any additional change requests. No problem, in my company they also "fear" my pedantic reviews 😄

[jo47011]

As for being pedantic: I can see that your code is not consistently formatted. Running black over the repo gives me 68 files reformatted. Mainly replacing single ticks with double ticks which are used interchangeably. But also some general formatting. If you want I can open a new PR once we have merged this one just w/ those changes. What do you think?

[mhthies]

As for being pedantic: I can see that your code is not consistently formatted. […] If you want I can open a new PR once we have merged this one just w/ those changes. What do you think?

I recently thought about replacing flake8 with ruff, which would also include the black functionality. So, I'm open to do this, but would suggest migrating to ruff directly.

We should also update the README file section "Tests and Code Style", when this is changed, and the CI script to check formatting with black/ruff.

Mainly replacing single ticks with double ticks which are used interchangeably.

I actually used both on purpose with some kind of system (single quotes for key-like string literals, double quotes for other strings), but I have to admit that this is not really consistent, there's lot of grey area and nobody else seems to use this convention. So I'm open to change this. :)

[jo47011]

Thanks for your approval. But I have no rights to merge the PR:

I just changed to ruff for my shc implementation. So that's a good match. I will put it on the list fort the shc repo. Will be a few days down the road 😉