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

Jump to bottom

Add selector-not-notation #5975

Merged
merged 1 commit into from Mar 27, 2022
Merged

Add selector-not-notation #5975

merged 1 commit into from Mar 27, 2022

Conversation

Mouvedia
Copy link
Contributor

Which issue, if any, is this issue related to?

#5974

Is there anything in the PR that needs further explanation?

read #5974 (comment)

Mouvedia
Mouvedia commented Mar 18, 2022
View reviewed changes
lib/rules/selector-not-notation/index.js
* @param {Node} node
* @returns {boolean}
*/
const isSimpleSelector = (node) =>
Copy link
Contributor Author

@Mouvedia Mouvedia Mar 18, 2022 •
edited

Choose a reason for hiding this comment

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

If another rule ever needs it, it will have to be moved to utils.
YAGNI

@Mouvedia Mouvedia mentioned this pull request Mar 18, 2022
Closed
jeddy3
jeddy3 requested changes Mar 18, 2022
View reviewed changes
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.

@Mouvedia Thank you! This is looking great already.

I've suggested some minor nits in the tests and docs, but the rule itself is shaping up really nicely.

lib/rules/selector-not-notation/README.md Show resolved Hide resolved
lib/rules/selector-not-notation/README.md Outdated Show resolved Hide resolved
lib/rules/selector-not-notation/README.md Outdated Show resolved Hide resolved
lib/rules/selector-not-notation/README.md Outdated Show resolved Hide resolved
lib/rules/selector-not-notation/README.md Outdated Show resolved Hide resolved
lib/rules/selector-not-notation/__tests__/index.js Outdated Show resolved Hide resolved
lib/rules/selector-not-notation/__tests__/index.js Outdated Show resolved Hide resolved
lib/rules/selector-not-notation/__tests__/index.js Outdated Show resolved Hide resolved
lib/rules/selector-not-notation/__tests__/index.js Outdated Show resolved Hide resolved
lib/rules/selector-not-notation/__tests__/index.js Outdated Show resolved Hide resolved
@Mouvedia Mouvedia force-pushed the mouvedia-not branch 4 times, most recently from 1237872 to 93fcf7b Compare March 19, 2022 20:11
@Mouvedia Mouvedia force-pushed the mouvedia-not branch 7 times, most recently from f911658 to 0c33e0e Compare March 20, 2022 09:05
@Mouvedia Mouvedia requested review from jeddy3 March 21, 2022 19:55
jeddy3
jeddy3 requested changes Mar 22, 2022
View reviewed changes
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.

@Mouvedia Thanks! It's looking great, especially the autofix.

I've requested some minor changes.

docs/user-guide/rules/list.md Outdated Show resolved Hide resolved
lib/rules/selector-not-notation/README.md Show resolved Hide resolved
lib/rules/selector-not-notation/index.js Show resolved Hide resolved
@jeddy3 jeddy3 mentioned this pull request Mar 24, 2022
Open
@Mouvedia Mouvedia force-pushed the mouvedia-not branch 2 times, most recently from a577466 to bbb6284 Compare March 24, 2022 23:46
jeddy3
jeddy3 approved these changes Mar 25, 2022
View reviewed changes
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 for making the changes.

LGTM, thank you!

(Pull requests that add rules need a second review, so we'll wait on one of those before merging, as the second pair of eyes usually catches something the first pair missed).

ybiquitous
ybiquitous reviewed Mar 25, 2022
View reviewed changes
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.

[suggest] What if making isNot() an assert function? Type-casting via @type will be unnecessary.

lib/rules/selector-not-notation/index.js Outdated Show resolved Hide resolved
lib/rules/selector-not-notation/index.js Outdated Show resolved Hide resolved
ybiquitous
ybiquitous reviewed Mar 25, 2022
View reviewed changes
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.

[suggest] .shift() can return undefined, so TS compiler complains. What if checking .shift() return value by assert() and removing @ts-ignore?

lib/rules/selector-not-notation/index.js Show resolved Hide resolved
lib/rules/selector-not-notation/index.js Outdated Show resolved Hide resolved
ybiquitous
ybiquitous reviewed Mar 25, 2022
View reviewed changes
lib/rules/selector-not-notation/index.js Outdated Show resolved Hide resolved
lib/rules/selector-not-notation/index.js Outdated Show resolved Hide resolved
ybiquitous
ybiquitous approved these changes Mar 27, 2022
View reviewed changes
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.

@Mouvedia Thank you. LGTM 👍🏼

@jeddy3 jeddy3 merged commit 7b0c541 into stylelint:main Mar 27, 2022
@jeddy3
Copy link
Member

jeddy3 commented Mar 27, 2022

Changelog entry added:

  • Added: selector-not-notation rule (#5975).

@Mouvedia Thanks again for contributing this rule.

@jeddy3
Copy link
Member

jeddy3 commented Mar 27, 2022

@ybiquitous Thanks for the 2nd review. I'm glad you reviewed as your type checking suggestions are always fantastic!

@Mouvedia Mouvedia deleted the mouvedia-not branch March 27, 2022 15:58
@sync-by-unito sync-by-unito bot mentioned this pull request May 1, 2022
Closed
@sync-by-unito sync-by-unito bot mentioned this pull request Jun 1, 2022
Closed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@jeddy3 jeddy3 jeddy3 approved these changes

@ybiquitous ybiquitous ybiquitous approved these changes

Assignees
No one assigned
Labels
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.

None yet
3 participants
@Mouvedia @jeddy3 @ybiquitous