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

#23253 Lint for valid CSS flags #31588

Conversation

DanielRyanSmith
Copy link
Contributor

@DanielRyanSmith DanielRyanSmith commented Nov 10, 2021

#23253
Running the lint will now check the CSS flags in the source file and check if they are valid or deprecated. If any are found that are invalid, the lint will now show an error and ask for a fix. This check is only done for test in the css/ directory.

CSS requirement flags pulled from the docs here.

@DanielRyanSmith
Copy link
Contributor Author

DanielRyanSmith commented Nov 15, 2021

It looks like the documentation build failing might be related to this issue here since the same warning is showing and we are using docutils-0.18. The lint check will not pass due to the new instances of invalid css requirement flags that are being found.

'should',
'svg'
}),
'deprecated': frozenset({
Copy link
Member

@foolip foolip Nov 17, 2021

Choose a reason for hiding this comment

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

How many of these do we have? Can they be removed instead of adding them to the lint?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Based on this run we have:
INFO:lint:There were 6907 errors (DEPRECATED-CSS-REQUIREMENT-FLAG: 1066 INVALID-CSS-REQUIREMENT-FLAG: 5841)
We can begin removing the invalid or deprecated flags as well. If we remove the existing flags, would it not be worth keeping a check in the lint to make sure no more invalid flags are added?

@DanielRyanSmith
Copy link
Contributor Author

This change will require a larger cleanup of the flags currently in the repository to be landed, and with that comes more work that will require further planning. This is a low-priority change with a high amount of work involved. Closing this for now.

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

Successfully merging this pull request may close these issues.

3 participants