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

feat: support svelte(prettier-plugin-svelte) and angular parser #156

Closed
wants to merge 1 commit into from
Closed

Conversation

JounQin
Copy link
Member

@JounQin JounQin commented Feb 22, 2021

sveltejs/prettier-plugin-svelte#210

close #25

@BPScott Please help to review.

@JounQin JounQin changed the title feat: support prettier-plugin-svelte feat: support prettier-plugin-svelte, fix test cases - close #157 Feb 22, 2021
@JounQin JounQin changed the title feat: support prettier-plugin-svelte, fix test cases - close #157 feat: support svelte(prettier-plugin-svelte) and angular parser, fix test cases Feb 22, 2021
@JounQin JounQin changed the title feat: support svelte(prettier-plugin-svelte) and angular parser, fix test cases feat: support svelte(prettier-plugin-svelte) and angular parser Feb 22, 2021
@@ -3,13 +3,22 @@ const {resolve} = require('path');
const stripAnsi = require('strip-ansi');

describe('E2E Tests', () => {
test('CSS/SCSS files', () => {
const result = runStylelint('*.{css,scss}');
test('CSS files', () => {
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 don't know why, but the order of the result are reversed sometimes, so I split the two type of files.

https://github.com/prettier/stylelint-prettier/runs/1954057143?check_suite_focus=true#step:7:9

Copy link
Member

Choose a reason for hiding this comment

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

Huh, that is annoying. I've addressed this separately so this PR can focus on the svelte change: #159

Rebase atop latest master and you should be able to remove this.

Copy link
Member

@BPScott BPScott left a comment

Choose a reason for hiding this comment

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

Thanks for the PR!

It does a couple of things unrelated to svelte/angular support, and I've pulled out avoiding flaky tests and testing against prettier v2 into separate PRs.

In light of those changes to help you focus this PR, can you:

  • Rebase atop latest master
  • Remove the flaky tests fixes as they're already in master
  • Remove the prettier v2 ci and the "endOfLine" changes to get that working, as they're already in master (but with a different approach, you shouldn't need to add endOfLine into any prettierrc files)
  • Check if renaming the "angular" test fixture file to check.inert.component.html will trigger the angular parser and if so, remove the override for "angular.html" files from .prettierrc

@@ -1,4 +1,3 @@
package.json
Copy link
Member

Choose a reason for hiding this comment

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

Why does this need to be removed? I would expect that the format of package.json is automatically handled by yarn/npm rather than being something that prettier needs to be concerned about

@@ -2,5 +2,14 @@
"arrowParens": "always",
"singleQuote": true,
"trailingComma": "es5",
"bracketSpacing": false
"bracketSpacing": false,
"endOfLine": "auto",
Copy link
Member

Choose a reason for hiding this comment

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

This can be removed, thanks to the more targeted fix in #158

@@ -7,14 +7,18 @@ on:

jobs:
ci:
name: 'Test: Node ${{ matrix.node-version }} - Stylelint ${{ matrix.stylelint-version }}'
name: 'Test: Node ${{ matrix.node-version }} - Prettier ${{ matrix.prettier-version }} - Stylelint ${{ matrix.stylelint-version }}'
Copy link
Member

Choose a reason for hiding this comment

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

I've split this out into a separate commit in #158, where I add Prettier to the test matrix and perform a more targeted fix for the "endOfLine" stuff.

Rebase and you should be able to remove the changes to this file, and all instances where you add "endOfLine": "auto" to configs.

@@ -39,6 +48,15 @@ check.invalid.scss
expect(result.output).toEqual(expectedResult);
expect(result.status).toEqual(0);
});

test('Svelte files', () => {
Copy link
Member

Choose a reason for hiding this comment

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

Svelte files get treated the same as HTML/Markdown/Vue files in that they are inert. Rather than add a new test case just for Svelte, let's add them to the existing "HTML/Markdown/Vue files" test case by updating the name and changing like 23 to be const result = runStylelint('*.{html,md,vue,svelte}');.

@@ -3,13 +3,22 @@ const {resolve} = require('path');
const stripAnsi = require('strip-ansi');

describe('E2E Tests', () => {
test('CSS/SCSS files', () => {
const result = runStylelint('*.{css,scss}');
test('CSS files', () => {
Copy link
Member

Choose a reason for hiding this comment

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

Huh, that is annoying. I've addressed this separately so this PR can focus on the svelte change: #159

Rebase atop latest master and you should be able to remove this.

@@ -1,5 +1,6 @@
{
"singleQuote": false,
"tabWidth": 4,
"trailingComma": "all"
"trailingComma": "all",
"endOfLine": "auto"
Copy link
Member

Choose a reason for hiding this comment

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

This can be removed, thanks to the more targeted fix in #158

@@ -2,6 +2,7 @@
"singleQuote": true,
"tabWidth": 2,
"trailingComma": "es5",
"endOfLine": "auto",
Copy link
Member

Choose a reason for hiding this comment

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

This can be removed, thanks to the more targeted fix in #158

@@ -0,0 +1,16 @@
<!DOCTYPE html>
Copy link
Member

Choose a reason for hiding this comment

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

https://github.com/prettier/prettier/blob/ce26805e84756953875f30a0c165bd6b09c94ea0/src/language-html/index.js#L13 suggests that you can trigger the angular parser by using the .component.html extension.

If we name this file check.inert.component.html then it looks like we should trigger using the angular component without needing to add any of the overrides in our base prettierrc.

"bracketSpacing": false
"bracketSpacing": false,
"endOfLine": "auto",
"overrides": [
Copy link
Member

Choose a reason for hiding this comment

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

By naming our fixture file check.inert.component.html we should be able to trigger the angular parser without needing to add this custom override. Can you confirm if that works and if so rename the fixture file and remove this?

@BPScott
Copy link
Member

BPScott commented Feb 28, 2021

As I was feeling on a roll I've addressed my concerns in #160 rather than making you do it all.

Thanks for addressing this in the first place though, your investigation made my work a lot easier!

I've published stylelint-prettier 1.2.0 with support for the angular and svelte parsers.

I'm going to close this PR as I think the root issue has been addressed. If you're still having issues please reopen with a set of changes against latest master.

@BPScott BPScott closed this Feb 28, 2021
@JounQin JounQin deleted the patch-1 branch February 28, 2021 04:13
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Style attributes in HTML being flagged as errors
2 participants