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 for no-duplicate-* #6047

Merged
merged 2 commits into from May 9, 2022
Merged

Conversation

mattxwang
Copy link
Member

@mattxwang mattxwang commented Apr 29, 2022

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

Part of the umbrella issue #5694.

Is there anything in the PR that needs further explanation?

I have a couple of questions on how to implement the end positions for these two rules, mostly due to my lack of experience. Would like some opinions!

For no-duplicate-at-import-rules:

  • what should be the start and end position? The rejection message just has the URI, but the README contains the entire at rule string (i.e. @import ...). I've currently opted for the latter, since it seems more useful for editor support.
  • if I should be implementing the latter, is writing atRule.toString() correct? I feel a bit strange writing this. Should I be recreating it with raws instead?

For no-duplicate-selectors:

  • currently, I have implemented the start/end positions that break the tests the least; namely, that we highlight the first of the duplicated selectors. Is this the correct method, or should I be doing something else? For example, reporting each instance of the duplicate selectors, or the last one?
  • I had to adjust a test (see below) - is my adjustment correct?

Once these are clarified, I can add endLine and endColumn to each of the test cases for standard CSS syntax.

Unsure of what the highlighted positions should be / what's the most
useful for the VSCode extension. Right now, it just highlights the
first duplicate selector. I believe this is because of the
implementation of the `word` parsing. Is it more useful to highlight
each duplicate selector? The last one? etc.
Two concerns:
- not sure if calling `atRule.toString()` is the appropriate use;
  should I be using `raws`?
- in this case, my suggested start/end positions are larger than the
  message rejection, but it aims to match the positions indicated
  in the README
@@ -81,8 +85,10 @@ testRule({
warnings: [
{
message: messages.rejected('a', 1),
line: 1,
line: 2,
Copy link
Member Author

Choose a reason for hiding this comment

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

I believe this line was previously incorrect; both a selectors are on lines 2 and 3 respectively, so I'm not sure how 1 would be reported.

If that is true, what does this mean for the test process? Is it because we didn't report a start index either?

@mattxwang mattxwang changed the title draft: fix end positions for no-duplicate-* Fix end positions for no-duplicate-* Apr 29, 2022
@mattxwang mattxwang marked this pull request as ready for review April 30, 2022 01:56
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.

@mattxwang Thank you. Your implementation has no problems 👍🏼

Maybe, we could improve end positions for no-duplicate-selectors (see #6075), but this pull request is enough for now.

@ybiquitous ybiquitous merged commit 6d87578 into main May 9, 2022
@ybiquitous ybiquitous deleted the end-positions-no-duplicate branch May 9, 2022 12:59
@ybiquitous
Copy link
Member

Changelog entry added:

Fixed: no-duplicate-* end positions (#6047).

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