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.

Sign up for GitHub

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 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 •
edited by jeddy3
Loading

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 self-assigned this Nov 19, 2021
@adalinesimonian adalinesimonian mentioned this pull request Nov 19, 2021
Closed
@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
Copy link
Member

jeddy3 commented Nov 21, 2021 •
edited
Loading

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

@adalinesimonian adalinesimonian mentioned this pull request Nov 22, 2021
Merged
@adalinesimonian
chore: update postcss dependency
a447e4d
@adalinesimonian
feat: report ranges for brace space rules
ec0aae0
@adalinesimonian
feat: report ranges for at rule space rules
2dc1792
@adalinesimonian
feat: report ranges for block-no-empty
5165d31
@adalinesimonian
feat: report ranges for color-named
42623c2
@adalinesimonian
feat: report ranges for comment-* rules
7cfd9a1
@adalinesimonian
feat: report ranges for custom-* rules
783d7ab
@adalinesimonian
feat: report ranges for declaration bang rules
8ca7b4f
@adalinesimonian
feat: report ranges for declaration-block-* rules
219f245
@adalinesimonian
feat: report ranges for declaration-colon-* rules
e1fc99d
@adalinesimonian
feat: report ranges for declaration-no-important
cf16178
@adalinesimonian
feat: report ranges for declaration-property rules
86a5976
@adalinesimonian
feat: report ranges for font-family rules
987a1d0
@adalinesimonian
feat: report ranges for font-weight-notation
9f7902b
@adalinesimonian
fix: correctly report ranges for declaration-block
97a9af6
@adalinesimonian
feat: report ranges for function-allowed-list
84315b9
@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.)

jeddy3
jeddy3 requested changes Apr 10, 2022
View reviewed changes
Copy link
Member

@jeddy3 jeddy3 left a comment •
edited
Loading

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
Closed
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.

jeddy3
jeddy3 approved these changes Apr 11, 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.

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).

@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
@niksy niksy mentioned this pull request Mar 3, 2023
Merged
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@ybiquitous ybiquitous ybiquitous left review comments

@jeddy3 jeddy3 jeddy3 approved these changes

Assignees

@adalinesimonian adalinesimonian

Labels
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
3 participants
@adalinesimonian @jeddy3 @ybiquitous