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

Stylelint Format Message Text #115

Merged
merged 1 commit into from
Feb 6, 2024
Merged

Conversation

bert-mccutchen
Copy link
Contributor

Fixes #112 ... again

Why?

Turns out that Stylelint doesn't pre-escape strings before they are passed to formatters, which I suppose makes sense. This was causing lots of invalid characters (like newlines) to make its way into the resulting JSON, which was breaking Reviewdog.

I am astounded at how JSON.stringify doesn't automatically escape invalid characters, and that it would rather produce an invalid JSON result.

Also, i'm not sure how this composite action was working before, since it was using the default JSON formatter - which doesn't do anything we needed to do here today. Maybe jq could parse the invalid JSON and then produce a valid result?

What

Added formatMessageText from the Stylelint stringFormatter source.

Copy link
Contributor

github-actions bot commented Feb 6, 2024

🏷️ [bumpr] Next version:v1.21.0 Changes:v1.20.2...reviewdog:stylelint-format-message-text

@bert-mccutchen
Copy link
Contributor Author

As a side note from all of this, we should add a more complete test suite that we can run via actions:

  1. Create a test file that breaks each rule for stylelint-config-recommended.
    • Also throw in newlines and special characters throughout.
  2. Check the Reviewdog output for the expected results.
  3. Run this for the following matrix:
    • Stylelint 15 - Node 14, 16, 18, 20
    • Stylelint 16 - Node 18, 20

@bert-mccutchen
Copy link
Contributor Author

Another thing we should do is create a separation between Stylelint 15 for CommonJS and Stylelint 16+ for ESM. This way we don't have to worry when they drop Stylelint 17 without CommonJS.

Copy link
Member

@haya14busa haya14busa 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 for your work👍

Adding tests sounds good.

@haya14busa haya14busa merged commit 5a62a0c into master Feb 6, 2024
6 checks passed
Copy link
Contributor

github-actions bot commented Feb 6, 2024

🚀 [bumpr] Bumped! New version:v1.21.0 Changes:v1.20.2...v1.21.0

@bert-mccutchen bert-mccutchen deleted the stylelint-format-message-text branch February 6, 2024 23:00
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Failed to unmarshal rdjson on version 1.20.0
2 participants