Replace properties with methods#22
Conversation
| @log_to_debug(setter=True) | ||
| def is_enabled(self, value: bool) -> None: | ||
| @log_to_debug | ||
| def power_on(self) -> None: |
There was a problem hiding this comment.
Here we have power_on, but the LED has on. Should we standardize?
|
Apologies for the drive by comment, I have no context for these changes.
Keen to understand where this has come from, is this feedback we've had from competitors or team supervisors? Every time we change the API we decrease (or in this case reset) the knowledge of existing volunteers. This in turn increases the dependency on a few volunteers who are keeping up to date with kit development to provide support. Given our very limited pool of active volunteers I think this should be a strong consideration. Likewise any first year competitors continuing into their second year that have learnt the old API will have to relearn this new API. I'm personally not sure we can give a strong enough explanation as to why we're making them do this. (This forced re learning could be considered a feature by some if we're aiming to reduce code reuse between years, but I think we should be focussing on uplifting new teams rather than hindering returning teams) In my mind V5 kit could be a sensible time to introduce such a huge breaking change like this. |
|
|
||
| @colour.setter | ||
| def colour(self, value: tuple[bool, bool, bool]) -> None: | ||
| def ste_colour(self, value: tuple[bool, bool, bool]) -> None: |
There was a problem hiding this comment.
| def ste_colour(self, value: tuple[bool, bool, bool]) -> None: | |
| def set_colour(self, value: tuple[bool, bool, bool]) -> None: |
Interesting that tests didn't pick up on this.
2 participants
Properties can be quite surprising for new users, especially if they have side effects.
This PR replaces most properties with getter / setter methods, and in some places simplifying the naming where it makes sense.