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

Improve eslint formatting #1961

Closed
wants to merge 16 commits into from

Conversation

hgiesel
Copy link
Contributor

@hgiesel hgiesel commented Jul 16, 2022

After this PR, prettier would be executed as part of eslint via eslint-plugin-prettier.

This would mean that, we need to run one "testing tool" less (eslint and svelte-check).

The only thing I stumbled upon is that prettier seems to also be used for formatting files in /docs? However they don't seem to actually be executed? Simply running prettier --write within the directory does change them.

Background: I worked on my tooling a bit, and configured running eslint --fix on save.

@dae
Copy link
Member

dae commented Jul 17, 2022

(I presume this is still a draft at this point)

I see the appeal of being able to run one fix command instead of having to run two separate ones. One downside of this change is that fixing formatting is slower now, though if the editor is formatting on the fly, I guess it won't be a problem.

After you get the build/test failures fixed, it would be interesting to run time bazel test //... --cache_test_results=no before and after this change to see what sort of speed benefit we gain from it.

The -svelte plugin receives 2k weekly downloads vs the offical one's 100k, which makes me a bit nervous about its long term support, but by the looks of the changes you've made so far, it seems to be doing a better job than the official one.

@hgiesel
Copy link
Contributor Author

hgiesel commented Jul 25, 2022

The -svelte plugin receives 2k weekly downloads vs the offical one's 100k

The numbers are a bit skewed, because the npm package just changed. This is the deprecated old entry, which still received 10k downloads this week:
https://www.npmjs.com/package/@ota-meshi/eslint-plugin-svelte

Maybe we'll have some kind of official statement at some point.

@JounQin
Copy link

JounQin commented Jul 26, 2022

Hi, I'm the active maintainer of https://github.com/prettier/eslint-plugin-prettier for now, I have to say the performance of it is a bit not idea although I'll improved it a lot at prettier/eslint-plugin-prettier#415 and prettier/eslint-plugin-prettier#485.

So for my own cases, I'm supporting env based choice at 1stG/configs#128, so that it can be chosen to prefer prettier cli or eslint-plugin-prettier instead locally or on CI.


Besides, for svelte, you should just use eslint-plugin-svelte instead of the official one.

See also https://github.com/prettier/eslint-plugin-prettier#svelte-support

@hgiesel hgiesel marked this pull request as draft July 26, 2022 22:06
@hgiesel
Copy link
Contributor Author

hgiesel commented Jul 26, 2022

Seems like using eslint-plugin-svelte does not cooperate with eslint-plugin-simple-sort-imports :sigh:

@JounQin
Copy link

JounQin commented Jul 27, 2022

Seems like using eslint-plugin-svelte does not cooperate with eslint-plugin-simple-sort-imports :sigh:

You could raise an issue with reproduction.

@dae
Copy link
Member

dae commented Jul 30, 2022

@dae
Copy link
Member

dae commented Mar 20, 2024

Cleaning up old PRs - please open a new one if you'd like to continue this.

@dae dae closed this Mar 20, 2024
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.

3 participants