Skip to content

Prevent subclassing of the Robot object#21

Merged
RealOrangeOne merged 1 commit intomainfrom
prevent-robot-subclass
Sep 9, 2025
Merged

Prevent subclassing of the Robot object#21
RealOrangeOne merged 1 commit intomainfrom
prevent-robot-subclass

Conversation

@RealOrangeOne
Copy link
Copy Markdown
Member

Teams will often try and subclass our robot to add their own functionality on top of it. Not only is it bad practice, but it can often conflict with the inner workings of our own classes.

Instead, teams should be encouraged to wrap our class. Guidance on that will come as a future docs PR. However for now, prevent subclassing.

@RealOrangeOne RealOrangeOne requested a review from a team June 3, 2025 22:10
@RealOrangeOne
Copy link
Copy Markdown
Member Author

I'm intentionally holding off merging this until we have relevant documentation to point teams in the right direction. Without that, I think this PR is just a hindrance.

@RealOrangeOne RealOrangeOne marked this pull request as draft June 5, 2025 08:24
@RealOrangeOne RealOrangeOne marked this pull request as ready for review September 9, 2025 19:24
@RealOrangeOne RealOrangeOne merged commit 4ee46a1 into main Sep 9, 2025
10 checks passed
@RealOrangeOne RealOrangeOne deleted the prevent-robot-subclass branch September 9, 2025 21:00
@PeterJCLaw
Copy link
Copy Markdown
Member

Are there some notes somewhere on why we're trying so hard to prevent people doing this? I'd like to better understand the motivations for this (not least because we'll need to also clearly communicate this to mentors).

Subclassing an API's class to extend it or modify its behaviour is a very common thing to do and is often explicitly the expected path. Discouraging it if we have specific reasons to do so may be fine, however the description here seems to be much more along the lines of the usual caveat emptor style stuff which is just the normal course of things.

I could understand a small warning in the docs which said something like "beware that when doing this you should be aware of these risks ..." and possibly a note on what specifically to look for, but the lengths being taken here (raising an exception when subclassing) seem rather extreme, especially given the relative usefulness of subclassing as an approach.

@WillB97
Copy link
Copy Markdown
Collaborator

WillB97 commented Sep 9, 2025

There's an accompanying docs update to explain the change here srobo/docs#677

@PeterJCLaw
Copy link
Copy Markdown
Member

There's an accompanying docs update to explain the change here srobo/docs#677

Thanks, however I'd seen that (I almost posted my comment there). The things that PR mentions are the sorts of things I alluded to which anyone subclassing should be aware of as risks, however they don't strike me as good reasons to directly discourage subclassing (which teams have been doing for a long time, usually without issues) nor put in place technical measures such as those in this PR.

FWIW: I can see value in suggesting the alternatives and making people aware of the risks, and even using the typing.final decorator. It's the stronger bits and raising an exception which I'm looking to better understand.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

4 participants