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 ranges for warnings #5725

Merged
merged 106 commits into from Apr 11, 2022
Merged

Add ranges for warnings #5725

merged 106 commits into from Apr 11, 2022

Conversation

adalinesimonian
Copy link
Member

@adalinesimonian adalinesimonian commented Nov 19, 2021

Partially addresses #5694

Adds range support for warnings.

Tasks:

  • Update PostCSS to 8.4
  • Update postcss-value-parser to 4.2
  • Update Warning type
  • Update Problem type
  • Update report function
  • Update some rules
  • Update tests for these rules

@adalinesimonian adalinesimonian added the type: enhancement a new feature that isn't related to rules label Nov 19, 2021
@adalinesimonian adalinesimonian self-assigned this Nov 19, 2021
@adalinesimonian
Copy link
Member Author

What this generally looks like in VS Code with the appropriate changes downstream:

Screenshot of VS Code reporting Stylelint warnings with ranges

@jeddy3
Copy link
Member

jeddy3 commented Nov 21, 2021

You're doing amazing work extending the capabilities of both PostCSS and Stylelint with this range stuff!

@jeddy3 jeddy3 removed the type: enhancement a new feature that isn't related to rules label Nov 21, 2021
@jeddy3
Copy link
Member

jeddy3 commented Nov 21, 2021

(Removed the label to be consistent with our usage, i.e. only pr: * labels for pull requests.)

@adalinesimonian
Copy link
Member Author

I was running into issues trying to determine the end position for functions and other nodes provided by postcss-value-parser. I've opened up a PR there that will resolve this problem: TrySound/postcss-value-parser#83

@ybiquitous
Copy link
Member

I've removed the unused utility functions below:

Let's add them again if they are necessary.

@ybiquitous ybiquitous marked this pull request as ready for review April 9, 2022 00:49
@ybiquitous
Copy link
Member

All the rules have not been supported yet, but this pull request is ready to review. 🎉
(we need to address the remaining rules on other pull requests.)

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.

@ybiquitous Thank you so much for picking this up! You've done fantastic work on top of @adalinesimonian's.

All the code LGTM. Let's update our docs too. I think we have a couple of examples that need updating here, here and here.

I think once that's done we can merge and release.

All the rules have not been supported yet, but this pull request is ready to review.

I agree. When it comes to the other rules, I don't think we need to add end positions for all the reject tests. We should add just enough of them to be confident that the extra endIndex logic is working.

@jeddy3 jeddy3 mentioned this pull request Apr 10, 2022
6 tasks
@ybiquitous
Copy link
Member

Ah, thanks for suggesting updating the docs. I'll do it soon. 👍🏼

@ybiquitous
Copy link
Member

Updating the docs was done via 13030a9.

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.

Fantastic work, thank you!

LGTM.

Feel free to merge. Any idea for the changelog entry? Perhaps:

  • Added: ranges for warnings that can be used by formatters and integrations

@ybiquitous
Copy link
Member

@jeddy3 Thanks for the review and suggestion! I'll do it.

@ybiquitous ybiquitous merged commit 1d94bb0 into main Apr 11, 2022
@ybiquitous ybiquitous deleted the warning-ranges branch April 11, 2022 08:35
@ybiquitous
Copy link
Member

Done.

  • Added: ranges for warnings that can be used by formatters and integrations (#5725).

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

3 participants