remove canvas adaptor protocol methods vis_set_width and vis_set_height

[alisterburt]

closes #28

[alisterburt]

@tlambert03 looks like the validate_backend_class is a little strict with expecting setters/getters for model fields - what do you think is the right path forwards here?

this feels very similar to the 'models with computed properties' and 'setters for computed properties' stuff we discussed a while ago

[tlambert03]

Ah right of course. So this is more of a model question really! Do we prefer height and width, or size? I think I might prefer height/width, with size being computed (I can never remember which comes first in the size and height/width serializes explicitly).

The evented model superclass does support the getters/setters, but you'd need to remove size from the model itself

[tlambert03]

Note that view also has height/width, and I think it's inconsistent right now (only having size)

[alisterburt]

yep I'm on board with height, width being the model attributes, will update here

[codecov[bot]]

Codecov Report

Merging #30 (acc07cd) into main (ca53558) will increase coverage by 0.10%. The diff coverage is 90.76%.

:exclamation: Current head acc07cd differs from pull request most recent head 5458dec. Consider uploading reports for the commit 5458dec to get more accurate results

@@            Coverage Diff             @@
##             main      #30      +/-   ##
==========================================
+ Coverage   67.24%   67.35%   +0.10%     
==========================================
  Files          26       26              
  Lines         922      925       +3     
==========================================
+ Hits          620      623       +3     
  Misses        302      302              

Impacted Files	Coverage Δ
src/microvis/backend/vispy/_camera.py	51.61% <0.00%> (ø)
src/microvis/backend/vispy/_canvas.py	63.63% <0.00%> (+0.77%)	:arrow_up:
src/microvis/core/_vis_model.py	93.18% <97.36%> (-0.73%)	:arrow_down:
src/microvis/core/canvas.py	100.00% <100.00%> (ø)
src/microvis/core/nodes/_data.py	86.00% <100.00%> (ø)
src/microvis/core/nodes/node.py	70.32% <100.00%> (ø)
src/microvis/core/view.py	98.21% <100.00%> (ø)

:mega: We’re building smart automated test selection to slash your CI/CD build times. Learn more

[alisterburt]

tried to figure out how to access the names of abstract methods on an adaptor class then realised... doesn't the fact that these methods are abstract mean we don't need to validate at runtime, the adaptor class wouldn't be able to instantiate itself if it didn't implement those methods right? I'm sure I'm missing something 😆

[tlambert03]

Did you happen to see this? https://github.com/alisterburt/microvis/pull/1

[alisterburt]

totally missed it! 🙈 will merge in now, thanks!

[alisterburt]

failures are my bad - found the bug I introduced, fixing now

[tlambert03]

I'm sure I'm missing something

The only thing you're "missing" is the (theoretical at this point) difference between a protocol and an ABC.

While all protocols are ABCs, the implication of a protocol is that one needn't actually subclass it (whereas you're right: with an actual abc.ABC you're generally using it as a superclass). Of course in this case we are currently always subclassing them.

But that's the difference: currently there's no assumption/requirement that these be subclassed. We could certainly say "you must subclass these protocols", (and that would be fine with me, I think we probably go overboard with the "you shouldn't need to import napari" thing) but that's what we'd be doing

[alisterburt]

gotcha, makes perfect sense - thanks for the explanation!

failures are my bad - found the bug I introduced, fixing now Thought I had it figured out but I don't - wasn't what I thought. I'm getting really weird behaviour with the tests

Running test_canvas or test_view individually they pass, running the whole test suite both fail... not sure what's going on and brain needs sleep for now, feel free to take a peek if interested

[tlambert03]

Thanks as always! It's a lot of fun having someone to talk about this with :)

[tlambert03]

ok, I'm happy enough with this now. reasonable "unit of work"... and I'm starting to touch too many things :joy:

[tlambert03]

(just realized pre-commit wasn't running on CI recently... so had to add a bunch of linting changes here too)