Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

fix: fix type of StatusBase subclasses by calling StatusBase.register in __init_subclass__ #1383

Conversation

james-garner-canonical
Copy link
Contributor

This is a simple alternative to #1382, following @dimaqq's suggestion to just not use StatusBase.register as a decorator. In this PR we instead do so via __init_subclass__.

@james-garner-canonical james-garner-canonical marked this pull request as ready for review September 20, 2024 06:03
@james-garner-canonical james-garner-canonical requested review from benhoyt, dimaqq and tonyandrewmeyer and removed request for benhoyt and dimaqq September 20, 2024 06:03
Copy link
Contributor

@tonyandrewmeyer tonyandrewmeyer left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I like it - I think there was a clear expectation that every subclass would register so doing it automatically, while a behaviour change, is ok.

I couldn't find any charms with (StatusBase, which isn't a perfect search for subclassing, but I think it's a good sign that no-one is doing that.

I like that register remains unchanged - we just ignore the returned class. So no need for deprecation warnings etc, and if people are registering different types of class (no idea why) then that would work. And we already allowed registering twice, so anyone doing this:

@Statusbase.register
class OtherStatus(StatusBase):
    name = "other"

Is still fine.

@james-garner-canonical
Copy link
Contributor Author

Thanks, @tonyandrewmeyer. I also tried ag 'class .*\(.*StatusBase' to see if anyone was subclassing StatusBase and didn't find anything.

If we wanted to deprecate register, we could move the functionality to _register, call that in __init_subclass__ and register, and add a deprecation warning to register. If there's interest in doing that, I'm happy to add it to the PR.

Copy link
Collaborator

@benhoyt benhoyt left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This approach is nice and simple -- looks good to me!

@james-garner-canonical james-garner-canonical merged commit 20c9623 into canonical:main Sep 22, 2024
29 checks passed
tonyandrewmeyer pushed a commit to tonyandrewmeyer/operator that referenced this pull request Oct 4, 2024
…bscuring subclass type (canonical#1383)

Previously, StatusBase.register obscured the type of decorated classes.
The classes would all end up typed as Type[StatusBase], and when instantiated
they would be typed as StatusBase instead of their actual class (see canonical#1380).
Instead we now register subclasses automatically in __init_subclass__,
no longer using StatusBase.register as a decorator, which avoids
obscuring the type of subclasses.
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.

3 participants