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.

Sign up for GitHub

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

Jump to bottom

Add new vue/enforce-style-attribute rule #2110

Merged
merged 13 commits into from
Jan 9, 2024
Merged

Add new vue/enforce-style-attribute rule #2110

merged 13 commits into from
Jan 9, 2024

Conversation

mussinbenarbia
Copy link
Contributor

This PR adds a new rule as proposed in this issue.

ota-meshi
ota-meshi requested changes May 7, 2023
View reviewed changes
Copy link
Member

@ota-meshi ota-meshi left a comment

Choose a reason for hiding this comment

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

See #2109 (comment)

@FloEdelmann FloEdelmann linked an issue May 16, 2023 that may be closed by this pull request
Rule Proposal: vue/enforce-style-attribute #2109
Closed
@mussinbenarbia
Copy link
Contributor Author

@ota-meshi Sorry for the very long absence 🙏 I made some modifications to the code to mimick how it's done in the eslint-plugin-vue-scoped-css plugin. Could you please take a look at the implementation? I have not updated the docs as I would like to make sure you agree with the implementation first.

Thank you 🙇

@ota-meshi ota-meshi marked this pull request as draft November 29, 2023 06:29
ota-meshi
ota-meshi requested changes Nov 29, 2023
View reviewed changes
Copy link
Member

@ota-meshi ota-meshi left a comment

Choose a reason for hiding this comment

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

That rule sounds good to me. Could you undraft this PR when it is ready for review?

@mussinbenarbia
Copy link
Contributor Author

@ota-meshi Thank you for taking a look! I will update the docs to reflect the new options and mark it as ready for review.

@mussinbenarbia mussinbenarbia marked this pull request as ready for review December 4, 2023 14:50
@mussinbenarbia
Copy link
Contributor Author

@ota-meshi (Looping in @FloEdelmann as well) I updated the docs but I have a question. When I run docs:watch or docs:build, the following piece of text gets automatically added to my doc file for the rule:

- :wrench: The `--fix` option on the [command line](https://eslint.org/docs/user-guide/command-line-interface#fixing-problems) can automatically fix some of the problems reported by this rule.

I'm not sure why. This rule does not have a fixer at the moment, so that paragraph is not required but it keeps being added automatically. Is it ok to remove it and push the code? Is that not going to make the CI fail?

@mussinbenarbia
Copy link
Contributor Author

Also, I'm now wondering if the allows option should actually be singular form allow to keep some form of consistency with other rules such as no-shadow 🤔 Do you agree?

FloEdelmann
FloEdelmann reviewed Dec 4, 2023
View reviewed changes
lib/rules/enforce-style-attribute.js Outdated Show resolved Hide resolved
lib/rules/enforce-style-attribute.js Outdated Show resolved Hide resolved
ota-meshi
ota-meshi reviewed Dec 27, 2023
View reviewed changes
Copy link
Member

@ota-meshi ota-meshi left a comment

Choose a reason for hiding this comment

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

Sorry for the late review.

docs/rules/enforce-style-attribute.md Outdated
{
"vue/enforce-style-attribute": [
"error",
{ "allow": ["scoped", "module", "no-attributes"] }
Copy link
Member

Choose a reason for hiding this comment

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

Hmm. I think the name no-attributes is misleading. lang="scss" has attributes. What do you think?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Yeah... that could be confusing 🤔 Maybe using plain like in the other plugin is the best choice here. If you agree I'll rename it

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 plain is better 👍

@mussinbenarbia
Copy link
Contributor Author

@ota-meshi I renamed the option to plain. I pushed the code but it seems some tests for other unrelated rules are failing. I would appreciate if you could take another look 🙇

@ota-meshi
Copy link
Member

Yeah. CI error should be fixed by #2357. So ignore it for now.

ota-meshi
ota-meshi approved these changes Jan 9, 2024
View reviewed changes
Copy link
Member

@ota-meshi ota-meshi left a comment

Choose a reason for hiding this comment

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

LGTM! Thank you!

@ota-meshi ota-meshi merged commit c232e26 into vuejs:master Jan 9, 2024
4 of 16 checks passed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@FloEdelmann FloEdelmann FloEdelmann left review comments

@ota-meshi ota-meshi ota-meshi approved these changes

Assignees
No one assigned
Labels
None yet
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.

Rule Proposal: vue/enforce-style-attribute
3 participants
@mussinbenarbia @ota-meshi @FloEdelmann