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

feat: add markup scopes to themelint #3983

Conversation

AlexanderBrevig
Copy link
Contributor

Based on the conversation here #3948 (comment) it would be nice if themelint also catches missing markup scopes.

I explicitly check all the sub keys, and not the catch all as to help people who omit the catch-all in favor of specificity.

@AlexanderBrevig AlexanderBrevig changed the title feat: add markup scopes feat: add markup scopes to themelint Sep 26, 2022
@kirawi kirawi added the S-waiting-on-review Status: Awaiting review from a maintainer. label Sep 26, 2022
@kirawi
Copy link
Member

kirawi commented Sep 26, 2022

We could also have themelint automatically run in CI.

Comment on lines +54 to +59
Require::Existence(Rule::has_either("markup.heading.1")),
Require::Existence(Rule::has_either("markup.heading.2")),
Require::Existence(Rule::has_either("markup.heading.3")),
Require::Existence(Rule::has_either("markup.heading.4")),
Require::Existence(Rule::has_either("markup.heading.5")),
Require::Existence(Rule::has_either("markup.heading.6")),
Copy link
Member

Choose a reason for hiding this comment

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

I think it's valid for a theme to only define markup.heading and not .marker or the .n for headings

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Defining markup.heading will make all these pass, but not defining it will force you to define everything.

Copy link
Contributor

Choose a reason for hiding this comment

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

If anything I think the error should be on markup.heading instead of .1... which is a bit misleading.

@pickfire
Copy link
Contributor

We could also have themelint automatically run in CI.

I am thinking we keep it off for now, or maybe make it faillable until we know what are the options we want to lint.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
S-waiting-on-review Status: Awaiting review from a maintainer.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants