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

Fix end positions in no-descending-specificity (and refactor index) #6049

Merged
merged 1 commit into from May 9, 2022

Conversation

mattxwang
Copy link
Member

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

Part of umbrella issue #5694.

Is there anything in the PR that needs further explanation?

I wanted to make a separate PR for this since I change part of the rule body.

I noticed that the only use of the index parameter for checkSelector was to generate the report; if we use the new word parameter, I think we don't need to manually calculate the index, etc. After removing this, the test behaviours haven't changed, but I'm not sure if I've missed any knock-on side effects (or if I'm inadvertently changing how duplicates are dealt with). Any thoughts welcome!

@ybiquitous
Copy link
Member

// The edge-case of duplicate selectors will act acceptably

I assume "The edge-case of duplicate selectors" looks like below:

a:hover {} a, b, a {}
/*         ↑     ↑
 */

In the case above, I think this rule reports the first a and the second a has different column positions.

@mattxwang
Copy link
Member Author

Gotcha. I think my change doesn't change the behaviour of the rule in that regard, so it should be good to review!

@ybiquitous
Copy link
Member

I understand the behavior doesn't change. 👍🏼

I'm concerned about the following edge-case:

{
	code: 'a:hover {} a, b, a {}',
	warnings: [
		{
			message: messages.rejected('a', 'a:hover'),
			line: 1,
			column: 12,
			endLine: 1,
			endColumn: 13,
		},
		{
			message: messages.rejected('a', 'a:hover'),
			line: 1,
			column: 18,    // but actual: 12
			endLine: 1,
			endColumn: 19, // but actual: 13
		},
	],
},

To address this case, we may need to iterate a selector node of postcss-selector-parser instead of ruleNode.selectors:

for (const selector of ruleNode.selectors) {

because we need an exact position of each selector to report a more accurate column position.

The implementation would be a bit too complex for the edge case.

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.

Thank you. LGTM 👍🏼

@mattxwang
Copy link
Member Author

mattxwang commented May 9, 2022

Thanks for the quick review @ybiquitous, and I think I understand now.

To combine what you've mentioned both here and in #6072 - should I write an issue and look to change our implementation(s) to use postcss-selector-parser in the long-term (even if it's not immediate)? Or should we keep it with what we have now.

@ybiquitous
Copy link
Member

@mattxwang Thanks for the suggestion. I think both PRs are ready to merge, but it would be helpful if you could open an issue. 👍🏼

@mattxwang
Copy link
Member Author

... it would be helpful if you could open an issue. 👍🏼

Thanks for the suggestion - have opened #6075, let me know what you think!

@ybiquitous
Copy link
Member

Thank you!

@ybiquitous ybiquitous merged commit 15b930c into main May 9, 2022
@ybiquitous ybiquitous deleted the end-positions-refactor-no-descending-specificity branch May 9, 2022 12:40
@ybiquitous
Copy link
Member

Changelog entry added:

Fixed: no-descending-specificity end positions (#6049).

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.

None yet

2 participants