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 no-irregular-whitespace #5209

Merged
merged 7 commits into from Apr 20, 2021

Conversation

itutto
Copy link
Contributor

@itutto itutto commented Mar 24, 2021

Implementation proposal for issue: 5201

This PR proposes an implementation for no-irregular-whitespace rule, which has been previously approved for implementation in the referenced issue.

Closes #5201

@ybiquitous
Copy link
Member

[question] In the blueprint #5201 (comment), this rule is proposed without secondary options, and I agree with the blueprint.
What reason do you have to provide the secondary options in this PR?

@itutto
Copy link
Contributor Author

itutto commented Mar 26, 2021

[question] In the blueprint #5201 (comment), this rule is proposed without secondary options, and I agree with the blueprint.
What reason do you have to provide the secondary options in this PR?

I thought it would be a nice addition if we gave the users some control over which types of irregular whitespaces should be disallowed. I don't have have a practical example where it definitely could come handy unless if we consider it may be purposefully used in an attribute selector.

a[title="some content with irregular whitespace"]

Please let me know if I should remove it anyway.

@ybiquitous
Copy link
Member

@itutto Thank you for the detailed description. I'm a bit worried if there is more complexity than needed... 🤔

For example, a case when both allow and only options are specified at the same time:

{
  "rules": {
    "no-irregular-whitespace": [
      true, { "allow": ["\u0085"], "only": ["\u00A0"] }
    ]
  }
}

But I would like to know @jeddy3's opinion about this.

lib/rules/no-irregular-whitespace/README.md Outdated Show resolved Hide resolved
lib/rules/no-irregular-whitespace/README.md Outdated Show resolved Hide resolved
lib/rules/no-irregular-whitespace/README.md Outdated Show resolved Hide resolved
lib/rules/no-irregular-whitespace/README.md Outdated Show resolved Hide resolved
lib/rules/no-irregular-whitespace/README.md Outdated Show resolved Hide resolved
lib/rules/no-irregular-whitespace/index.js Outdated Show resolved Hide resolved
lib/rules/no-irregular-whitespace/index.js Outdated Show resolved Hide resolved
lib/rules/no-irregular-whitespace/index.js Outdated Show resolved Hide resolved
lib/rules/no-irregular-whitespace/index.js Outdated Show resolved Hide resolved
lib/rules/no-irregular-whitespace/index.js Outdated Show resolved Hide resolved
@jeddy3
Copy link
Member

jeddy3 commented Mar 27, 2021

@itutto Thank you for the pull request. It's looking good so far.

Please let me know if I should remove it anyway.

Yes, please remove the secondary options. We only them when there has been a specific user request for them. There are lots of rules in stylelint and so we try to minimise complexity where we can.

- various updates based on pr review
- refactored tests
@itutto
Copy link
Contributor Author

itutto commented Mar 27, 2021

@itutto Thank you for the pull request. It's looking good so far.

Please let me know if I should remove it anyway.

Yes, please remove the secondary options. We only them when there has been a specific user request for them. There are lots of rules in stylelint and so we try to minimise complexity where we can.

Thank you both for the reviews. I've removed the secondary options and applied the other suggested changes as well.

Copy link
Member

@ybiquitous ybiquitous left a comment

Choose a reason for hiding this comment

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

@itutto Thanks for addressing reviews. It seems this PR is almost enough already, but I left some refactoring ideas.

It would be nice if you can check it. 😄

lib/rules/no-irregular-whitespace/index.js Outdated Show resolved Hide resolved
Copy link
Member

@ybiquitous ybiquitous left a comment

Choose a reason for hiding this comment

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

LGTM 👍

@itutto
Copy link
Contributor Author

itutto commented Apr 15, 2021

Would you like to see any further changes, @jeddy3 ?

Copy link
Member

@jeddy3 jeddy3 left a comment

Choose a reason for hiding this comment

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

Thanks! LGTM.

@jeddy3 jeddy3 merged commit 61fe25d into stylelint:master Apr 20, 2021
@jeddy3 jeddy3 changed the title new rule: no-irregular-whitespace Add no-irregular-whitespace Apr 20, 2021
@jeddy3
Copy link
Member

jeddy3 commented Apr 20, 2021

Changelog entry added:

  • Added: no-irregular-whitespace rule (#5209).

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

Successfully merging this pull request may close these issues.

Add no-irregular-whitespace
3 participants