Better inheritance and more

[teutoburg]

This solves the "grandfather super()" issue in the FilterWheel subclasses by introducing a FilterWheelBase base class, to separate code that applies to FilterWheel but not to subclasses (which isn't a lot anyway).

Inspired by the main conflict (different required keys) that led to this issue in the first place, I made some changes to how required_keys is handled: I made it a class attribute, and call check_keys afterwards. This is in preparation for a change I started thinking about some time ago: Move the check_keys call up in the inheritance, either as part of Effect or a new class in between. Since subclass attributes take priority above parent class attributes, that should work fine and remove the check_keys call from the subclasses. For now, I only changed things in the classes affected here anyway, in a backwards-compatible way, to keep this PR limited.

Additionally, I had noticed earlier that several Effect subclasses define a very similar display_name property (overriding that property from Effect). The only difference was the key used to get the name from currsys, usually some "current_xy". So for those classes, I introduced another class attribute _current_str, which holds that key (string), hardcoded. The (previously existing) property Effect.display_name now checks if that attribute exists and falls back to the previous behavior if that's not the case. So, all behaviors should stay the same, but with less code duplication.

Finally, in the same move, I also abstracted ._get_path() to Effect, because the very same line existed in at least 4 unrelated subclasses. Again, if the subclass in question does not have the required keys, nothing happens.

Opinions and comments on all of this are of course welcome 😄

[codecov]

Codecov Report

Patch coverage: 85.00% and project coverage change: +0.14% 🎉

Comparison is base (11442ac) 80.05% compared to head (44440ba) 80.20%.

Additional details and impacted files

@@              Coverage Diff               @@
##           dev_master     #264      +/-   ##
==============================================
+ Coverage       80.05%   80.20%   +0.14%     
==============================================
  Files             143      143              
  Lines           14777    14779       +2     
==============================================
+ Hits            11830    11853      +23     
+ Misses           2947     2926      -21     

Files Changed	Coverage Δ
scopesim/effects/spectral_efficiency.py	66.10% <0.00%> (ø)
scopesim/effects/surface_list.py	66.05% <0.00%> (ø)
scopesim/effects/ter_curves.py	83.07% <81.25%> (+1.57%)	⬆️
scopesim/effects/effects.py	90.21% <87.50%> (-0.38%)	⬇️
scopesim/effects/apertures.py	69.69% <100.00%> (-0.23%)	⬇️
scopesim/effects/spectral_trace_list.py	61.11% <100.00%> (+7.77%)	⬆️
...esim/tests/tests_effects/test_SpectralTraceList.py	96.72% <100.00%> (+0.64%)	⬆️
scopesim/tests/tests_effects/test_TERCurve.py	86.08% <100.00%> (ø)

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

[teutoburg]

From the coverage report, I noticed that SpectralTraceListWheel didn't seem to be covered by any test at all, at least non that I could find. I really don't know enough about that class and its intended use, so I couldn't write a super meaningful test for it. Anyway, I added a super basic test with some "bogus" values for the required keywords, just to make sure the __init__() runs at all, and parameters are passed alright.

If anyone feels confident to write more meaningful tests for this class, by all means please do so!

[hugobuddel]

This makes everything a bit better and more generic. So lets get this merged.

Filtercurve impression: ➰ ➰ ➰

About your future plans: sounds good. I'm not sure why the elements of meta aren't just attributes of the classes. Maybe the Effect classes could even be dataclasses. But if it ain't broken, we shouldn't fix it. And now it was kinda broken, and you took it a step in the right direction.