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

Add support for pseudo-classes in nested TCSS. #4252

Merged
merged 4 commits into from
Mar 5, 2024

Conversation

rodrigogiraoserrao
Copy link
Contributor

Fixes #4039 and supersedes #4163.

To be able to disambiguate between selector:pseudo-class and declaration:rule-value in nested TCSS (see #4039 for the original issue and #4163 for a first attempt at solving this) we establish that selectors with widget type names always start with an upper case letter A-Z or an underscore _ whereas declarations always start with a lower case letter a-z.

When a user creates a widget subclass that doesn't conform to this, we issue a 'SyntaxWarning' to let the user know. Because we do this with the standard module 'warnings', the warning can easily be supressed if the user has a good reason to create a widget subclass with a name starting with a lower case letter (which is valid Python, just unhelpful to Textual).

To be able to disambiguate between selector:pseudo-class and declaration:rule-value in nested TCSS (see #4039 for the original issue and #4163 for a first attempt at solving this) we establish that selectors with widget type names always start with an upper case letter A-Z or an underscore _ whereas declarations always start with a lower case letter a-z.
When a user creates a widget subclass that doesn't conform to this, we issue a 'SyntaxWarning' to let the user know.
Because we do this with the standard module 'warnings', the warning can easily be supressed if the user has a good reason to create a widget subclass with a name starting with a lower case letter (which is valid Python, just unhelpful to Textual).
@rodrigogiraoserrao rodrigogiraoserrao self-assigned this Mar 4, 2024
Comment on lines 267 to 273
if not name[0].isupper() and not name.startswith("_"):
warnings.warn(
SyntaxWarning(
f"Widget subclass {name!r} should be capitalised or start with '_'."
),
stacklevel=2,
)
Copy link
Contributor Author

Choose a reason for hiding this comment

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

Because this is from the std library, suppressing these warnings is “easy”.
Even so, do we add something like a Boolean textual.widget.SHOW_WIDGET_NAME_WARNING to make it even easier for users to suppress the warning?

Copy link
Contributor

Choose a reason for hiding this comment

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

(just looking at the code in the comment above; so might be missing context)

Is it intentional that _foo seen as valid? (I'm mostly guessing yes, but wanted to be sure).

Copy link
Contributor Author

Choose a reason for hiding this comment

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

_foo is valid and that's ok in the context in which this restriction was introduced.

Ideally, the wording of the warning (and of the comments around the code) should give that away.

I'm open to making it invalid as well if we think it makes sense to be a bit more restrictive with the goal of looking more coherent (given that foo raises the warning but Foo doesn't).

Copy link
Contributor

Choose a reason for hiding this comment

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

Nah, I think it's fine; just wanted to be 100% sure that the whole package of code was doing what motivated it.

Copy link
Collaborator

@willmcgugan willmcgugan left a comment

Choose a reason for hiding this comment

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

I think this is a good compromise. A few comments about the implementation.

Could I see screenshots of the error cases? And in particular the ones that weren't helpful prior to this...

src/textual/widget.py Outdated Show resolved Hide resolved
src/textual/widget.py Outdated Show resolved Hide resolved
src/textual/css/tokenize.py Show resolved Hide resolved
@rodrigogiraoserrao
Copy link
Contributor Author

Could I see screenshots of the error cases?

All error messages are what you would expect. There are tests that check we didn't regress & I added tests that check in nested contexts the messages are the same.

Here's a screenshot of the case that was unhelpful in the previous PR and which is now as good as it gets:

Screenshot 2024-03-05 at 10 46 03

@rodrigogiraoserrao rodrigogiraoserrao merged commit 826ffd4 into main Mar 5, 2024
20 checks passed
@rodrigogiraoserrao rodrigogiraoserrao deleted the nested-tcss-accept-pseudo-classes branch March 5, 2024 11:05
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.

Nested TCSS interacts poorly with pseudo-classes in selectors.
3 participants