feat: convert keybinding to normal class

[kne42]

this is an idea PR to convert keybinding to a normal class instead of a base model to allow it to be represented properly in json and yaml without complicated workarounds

[codecov]

Codecov Report

Merging #68 (547c857) into main (79bfc72) will not change coverage.
The diff coverage is 100.00%.

@@           Coverage Diff           @@
##             main      #68   +/-   ##
=======================================
  Coverage   99.34%   99.34%           
=======================================
  Files          31       31           
  Lines        1692     1692           
=======================================
  Hits         1681     1681           
  Misses         11       11           

Impacted Files	Coverage Δ
src/app_model/backends/qt/_qkeymap.py	100.00% <100.00%> (ø)
src/app_model/types/_keys/_keybindings.py	100.00% <100.00%> (ø)

Help us with your feedback. Take ten seconds to tell us how you rate us. Have a feature suggestion? Share it here.

[liu-ziyang]

@kne42 there are some failing checks. Can you please see if you can fix those?

[kne42]

the test that was failing was a codecov since it expects 100% coverage, however it lost coverage for indirect reasons and im not sure the proper way to cover it since it seems specific to pydantic, i tried removing it but apparently it is actually used, @tlambert03 any advice?

[tlambert03]

This seems like a very nice solution to the problem. as a "leaf" node of the model, this doesn't need to be BaseModel, so this is just fine by me!

the test that was failing was a codecov since it expects 100% coverage, however it lost coverage for indirect reasons and im not sure the proper way to cover it since it seems specific to pydantic, i tried removing it but apparently it is actually used, @tlambert03 any advice?

looks like the offending line is line 114 in _keybindings. And any fix here is fine:

I actually can simply remove the SimpleKeyBinding.__get_validators__ method and tests seem to pass just fine? not sure why you'd be seeing different results, but if so, you can either just use # pragma: no cover on that line ... or make a simple smoke test to assert that it returns the validate() method.

side note about test coverage. I do think it's great to keep coverage at very near 100% if possible... but I also think it's ok to be reasonable about it. If you occasionally come across a line where it just really seems like it's never going to be a problem, then I think it's totally fine to use #pragma: no cover. To me, having the 100% patch requirement is still good, and seeing that line doesn't always have to be a cop-out, it just says "I human have contemplated this and determined that it is never going to break and isn't work the effort". If it's simple to write a test (say, for a __repr__ or a __hash__ or something), then just go ahead and do it. it provides peace of mind later ... but also don't feel like you can't ever skip a line because I started it out that way 👍

[kne42]

Ah yeah, I guess I got confused and assumed that the offending line was on KeyBinding instead of SimpleKeyBinding since that's the class I changed, oops!

After this is merged, I want to create a release. Looking at the workflow, all I need to do is create and push a tag on main with v0.0.10, is that correct?

[tlambert03]

After this is merged, I want to create a release. Looking at the workflow, all I need to do is create and push a tag on main with v0.0.10, is that correct?

yes, but the changelog needs to be updated first. I do that using github changelog generator. I can do it later today, and then will try to make a workflow for it. (there's one on npe2 you could emulate)

[tlambert03]

ping me when everything is passing and merged here and you want a release

[kne42]

tests are failing due to an error in superqt: https://github.com/napari/superqt/pull/128 fixed

[tlambert03]

v0.1.0 is up!