search

gilesknap / maaspower

Provide MAAS power control via webhooks for various remote control power switches.
Apache License 2.0
18 stars 13 forks source link

fix #14: reduce number of mandatory fields in SwitchDevice #16

Closed rvodden closed 10 months ago

rvodden commented 11 months ago

This is a quite PR to resolve #14 as I was seeing exactly the same issue. I have removed all the Field from SwitchDevice except for name and description and migrated these down to the concrete classes. The Regex code would have been repeated across three of the concrete classes so I've added a class in the hierarchy called "RegexSwitchDevice" which contains this common code.

I've also done a couple of things which you may like me to back out, and I'd be happy to do so:

  1. I've knocked the minimum python version up to 3.10 as this version introduces the kw_only keyword to the dataclass decorator and greatly cleans up dataclass hierarchies. No longer is it necessary to have all the mandatory fields strictly before the optional fields throughout the tree.

  2. I've explicitly declared SwitchDevice and RegexSwitchDevice to be abstract classes by inheriting from abc.ABC - under the hood this is exactly the same as throwing a 'NotImplementedError' but it looked a little cleaner to my eye.

If you still have an appetite for it, I'd be happy to bash out a few of the other things on your todo list. I notice that you'd like to migrate away from print and instead use logging? That shouldn't take too long. I'd also be happy to take a look at fixing your SmartThing test.

gilesknap commented 10 months ago

Hey thanks for the interest. The changes look good and I approve of 1. and 2.

I'm not really using this repo much at the moment as I came to the conclusion that MAAS was too much overhead for my tiny cluster of 4 RASPIs - but I had fun making this and happy to collaborate on any further changes you are interested in.

You have PR CI permission now and I see that CI has failed. No time to look at this this evening but will take a look probably Sat morning.

Cheers, giles.

rvodden commented 10 months ago

Great stuff! We're making extensive use of MAAS and of course our PDUs aren't natively supported, so I'm happy to put in quite a bit of time to maaspower, it's been a real life saver.

CI is failing cause the docs arent building. I confess I didn't test that before I raised the PR. I'll push a fix ahead of Saturday so it will all be green by the time you get there.

rvodden commented 10 months ago

Documentation is working locally for me now - the pipenv lock file was out of date. I've pushed up the change, so hopefully the build will now succeed.

gilesknap commented 10 months ago

Hmmm, I thought that approving your PR for CI once would allow future pushes to run the CI without need of approval but that does not seem to have happened.

Maybe it works for further PRs.

I'm accepting this as the CI passed.

gilesknap commented 10 months ago

Thanks for the contribution.

gilesknap commented 10 months ago

@rvodden I've just noticed that the CI failed on this. Mainly a python version thing.

I think we should drop support for 3,8 but need to look at why 3.12 is failing and also 3.12 will be the cause of lint failing

rvodden commented 10 months ago

Ah. I'm a little busy tonight, but I can take a look tomorrow morning.

gilesknap commented 10 months ago

great, thanks!