prettier / prettier-eslint

Code :arrow_right: prettier :arrow_right: eslint --fix :arrow_right: Formatted Code :sparkles:
http://npm.im/prettier-eslint
MIT License
3.96k stars 171 forks source link

feat: support handling `.svelte` files #950

Closed robertheessels closed 5 months ago

robertheessels commented 5 months ago

Without this addition, prettier-eslint will only run the Prettier step for .svelte files, but NOT the Eslint step!

close #525

changeset-bot[bot] commented 5 months ago

🦋 Changeset detected

Latest commit: 1791e37eef136d18dfe7416f220da1e181c0bab9

The changes in this PR will be included in the next version bump.

This PR includes changesets to release 1 package | Name | Type | | --------------- | ----- | | prettier-eslint | Minor |

Not sure what this means? Click here to learn what changesets are.

Click here if you're a maintainer who wants to add another changeset to this PR

JounQin commented 5 months ago

Codes look good to me, CI is broken, maybe a eslint-disable is required.

robertheessels commented 5 months ago

I'm new to this repo. Does the lint problem have anything to do with my pull request at all?

JounQin commented 5 months ago

I'm new to this repo. Does the lint problem have anything to do with my pull request at all?

The CI is broken because the new codes add one more complexity. See

[lint] /home/runner/work/prettier-eslint/prettier-eslint/src/index.js
[lint]   59:1  warning  Async function 'analyze' has a complexity of 14. Maximum allowed is 13  complexity
[lint] 
[lint] ✖ 1 problem (0 errors, 1 warning)

https://github.com/prettier/prettier-eslint/actions/runs/7572175398/job/20622456917?pr=950#step:5:17

JounQin commented 5 months ago

@robertheessels Test case is missing, you can follow https://github.com/prettier/prettier-eslint/blob/689f67ab05a5146aa756cf4ef49d4e92dc6b293e/src/__tests__/index.js#L182-L195 to add a new svelte test case.

robertheessels commented 5 months ago

@JounQin I added the test an hour ago (even before your comment 😄), but the ci still seems to be in queue or something? ci (18) Expected — Waiting for status to be reported

JounQin commented 5 months ago

https://github.com/prettier/prettier-eslint/actions/runs/7582808184/job/20654893335?pr=950

@robertheessels

You'll need to add https://github.com/sveltejs/prettier-plugin-svelte at https://github.com/prettier/prettier-eslint/blob/master/.prettierrc.js

Could you please run the test cases successfully before committing?

I believe it should be added as an optional dependency.

robertheessels commented 5 months ago

@JounQin Sorry for not running test locally, doing so now.

Is .prettierrc.js even used in the tests? I have tried adding prettier-plugin-svelte to it, as I have done successfully in my other project, and in a few other different ways, but always it errors with No parser could be inferred.

Even if I add invalid strings in .prettierrc.js, the tests run without complaining about that, so I was wondering if .prettierrc.js is used in the tests?

JounQin commented 5 months ago

It should be related to prettier.resolveConfig is mocked on testing at https://github.com/prettier/prettier-eslint/blob/689f67ab05a5146aa756cf4ef49d4e92dc6b293e/src/__mocks__/prettier.js#L10

You can change it just like mockFormat to be a wrapper

robertheessels commented 5 months ago

@JounQin All tests passing locally. I had to add more packages than I expected, like svelte itself. Not sure if that is desirable or acceptable. But this way prettier-eslint can now handle Svelte files.

I believe it should be added as an optional dependency.

I have no idea how to do that?

codecov-commenter commented 5 months ago

Codecov Report

All modified and coverable lines are covered by tests :white_check_mark:

Comparison is base (689f67a) 100.00% compared to head (1791e37) 100.00%.

Additional details and impacted files ```diff @@ Coverage Diff @@ ## master #950 +/- ## ========================================= Coverage 100.00% 100.00% ========================================= Files 2 2 Lines 301 303 +2 Branches 83 84 +1 ========================================= + Hits 301 303 +2 ```

:umbrella: View full report in Codecov by Sentry.
:loudspeaker: Have feedback on the report? Share it here.

JounQin commented 5 months ago

@robertheessels

I believe it should be added as an optional dependency.

I have no idea how to do that?

peerDependencies

peerDependenciesMeta

robertheessels commented 5 months ago

@JounQin

If I put any of my 3 packages in peerDependencies, the test will fail, as npm i will not install them. I have now put the in optionalDependencies, which works, as npm will now install them. Or should we change the npm i in ci.yml?

I have no experience at all with peerDependencies and optionalDependencies, so I really don't know what is best here?

JounQin commented 5 months ago

It's peerDependencies and peerDependenciesMeta, not optionalDependencies, as I linked above.

I'll edit this PR later when I'm free then.

robertheessels commented 5 months ago

It's peerDependencies and peerDependenciesMeta, not optionalDependencies, as I linked above.

I'll edit this PR later when I'm free then.

See my last comment. :-)

JounQin commented 5 months ago

I don't understand, you were saying optionalDependencies, I'm saying peerDependenciesMeta?

Both peerDependencies and devDependencies should be used. (svelte should not be peer.)

JounQin commented 5 months ago

@robertheessels Thanks for your contribution!

robertheessels commented 5 months ago

@JounQin You're welcome!

Did the tests run in CI without the svelte package? They failed for me locally without it.

JounQin commented 5 months ago

@robertheessels It's already been listed in devDependencies.

robertheessels commented 5 months ago

Right.

Once a new prettier-eslint version is released, I can update the docs, if you want. We should tell users about the Svelte option? And maybe also suggested Prettier and Eslint configs for Svelte?

JounQin commented 5 months ago

Oh, sure, I totally forgot about the document part, feel free to raise a new PR.