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

The type of dark attribute of class App might be inaccurate. (Reactive[bool] -> bool) #4614

Closed
tongl2 opened this issue Jun 6, 2024 · 5 comments

Comments

@tongl2
Copy link

tongl2 commented Jun 6, 2024

Current code in src/textual/app.py:

    dark: Reactive[bool] = Reactive(True, compute=False)

I personally believe the type of the attribute "dark" should be bool instead of Reactive. (Correct me if I was wrong.)

Reactive class does not implement __bool__ method, so Reactive(False) is actually True when casted to bool, which is quite confusing.
In the code example, the first execution of self.app.dark = not self.app.dark is actually setting self.app.dark to boolean type False, which mean if I initially set self.app.dark to Reactive(False), the theme is still light unless I toggle it once.
The code seems not using features of Reactive on dark attribute, so it will be more convincing changing the type to bool.

Copy link

github-actions bot commented Jun 6, 2024

Thank you for your issue. Give us a little time to review it.

PS. You might want to check the FAQ if you haven't done so already.

This is an automated reply, generated by FAQtory

@tongl2
Copy link
Author

tongl2 commented Jun 6, 2024

Directly setting self.dark to False will raise a type-check lint:
image

@davep
Copy link
Contributor

davep commented Jun 6, 2024

Dark should be Reactive as it is a reactive. Moreover, setting dark to False has the desired effect with this code:

from textual.app import App, ComposeResult
from textual.widgets import Label

class LightApp(App[None]):

    def __init__(self):
        super().__init__()
        self.dark = False

    def compose(self) -> ComposeResult:
        yield Label("This is a light app")

if __name__ == "__main__":
    LightApp().run()

I also see no type issues with the code (in this case using pyright):

Screenshot 2024-06-06 at 14 54 24

It might be a good idea to say what you're using for type checking, and perhaps which IDE you're using. For example, the type checker in (some versions of?) PyCharm are known to be lacking a little and sometimes can mislead people.

@willmcgugan
Copy link
Collaborator

Yeah, PyCharm doesn't understand descriptors all that well. @Haphaistos2333 feel free to report this to the PyCharm folks.

Copy link

github-actions bot commented Jun 6, 2024

Don't forget to star the repository!

Follow @textualizeio for Textual updates.

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

No branches or pull requests

3 participants