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

HelpTheme as dataclass rather than NamedTuple #163

Merged
merged 1 commit into from
Jul 14, 2023

Conversation

janluke
Copy link
Owner

@janluke janluke commented Jul 14, 2023

@janluke janluke added the enhancement New feature or request label Jul 14, 2023
@kdeldycke
Copy link
Contributor

I tried to directly inherits from your new HelpTheme dataclass, but of course this is not allowed:

TypeError: cannot inherit non-frozen dataclass from a frozen one 😅

@janluke
Copy link
Owner Author

janluke commented Jul 14, 2023

Do you need it to be a non-frozen dataclass?

@kdeldycke
Copy link
Contributor

Do you need it to be a non-frozen dataclass?

Unfreezing it would allow me to streamline my code with:

@dataclass
class HelpExtraTheme(cloup.HelpTheme):

    critical: IStyle = identity
    error: IStyle = identity
    warning: IStyle = identity
    info: IStyle = identity
    debug: IStyle = identity
    """Log levels from Python's logging module."""

    ...

Else I have to build up a new class programmatically out of cloup.HelpTheme fields.

Freezing that dataclass is tempting and I understand the appeal. But I guess that's not critical. I'm probably the only one on the planet trying to play with the internals of cloup.HelpTheme. So no need to protect me from myself! 😇 I'll take full responsibility and not blame you if I mess up! 😁

@janluke
Copy link
Owner Author

janluke commented Jul 14, 2023

Sorry, it's still not clear to me why you can't

@dataclass(frozen=True)
class HelpExtraTheme(cloup.HelpTheme):
    ...

@kdeldycke
Copy link
Contributor

Sorry, it's still not clear to me why you can't

Because I'm stupid! 😓

I apologize for the back and forth, I'm just discovering this implementation detail. You are right: it is indeed possible to subclass a frozen dataclass. All you need to make it frozen too. And mine wasn't.

Here is the proof:

Python 3.11.4 (main, Jun 20 2023, 17:23:00) [Clang 14.0.3 (clang-1403.0.22.14.1)] on darwin
Type "help", "copyright", "credits" or "license" for more information.

>>> from dataclasses import dataclass

>>> @dataclass(frozen=True)
... class HelpTheme:
...     ...
... 

>>> HelpTheme
<class '__main__.HelpTheme'>

>>> @dataclass
... class HelpExtraTheme(HelpTheme):
...     ...
... 
Traceback (most recent call last):
  File "<stdin>", line 1, in <module>
  File ".../python3.11/dataclasses.py", line 1230, in dataclass
    return wrap(cls)
           ^^^^^^^^^
  File ".../python3.11/dataclasses.py", line 1220, in wrap
    return _process_class(cls, init, repr, eq, order, unsafe_hash,
           ^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^
  File ".../python3.11/dataclasses.py", line 988, in _process_class
    raise TypeError('cannot inherit non-frozen dataclass from a '
TypeError: cannot inherit non-frozen dataclass from a frozen one

>>> @dataclass(frozen=True)
... class HelpExtraTheme(HelpTheme):
...     ...
... 

>>> HelpExtraTheme
<class '__main__. HelpExtraTheme'>

Just tried this branch against my code and it works! 🎉

See: https://github.com/kdeldycke/click-extra/pull/704/files

So yeah, you can merge this PR. LGTM!

@kdeldycke
Copy link
Contributor

Also, I had to redefine the with_ helper. My version is a little convoluted but generic. Feel free to adopt it if it makes sense for Cloup: https://github.com/kdeldycke/click-extra/blob/f93cbc8ddfa8b8cc135f54474929bd74cf57ec7f/click_extra/colorize.py#L87-L109

@janluke janluke merged commit b407fb2 into master Jul 14, 2023
@janluke janluke deleted the helptheme-as-dataclass branch July 14, 2023 14:05
@janluke janluke added the breaking change This breaks backward compatibility label Jul 14, 2023
@kdeldycke
Copy link
Contributor

Thanks for the merge! 🎉

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
breaking change This breaks backward compatibility enhancement New feature or request
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Change HelpTheme type to allow sub-classing
2 participants