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

Linters integration discussion #922

Closed
risen228 opened this issue Aug 24, 2019 · 16 comments
Closed

Linters integration discussion #922

risen228 opened this issue Aug 24, 2019 · 16 comments
Labels
discussion locked Please open a new issue and fill out the template instead of commenting.

Comments

@risen228
Copy link

risen228 commented Aug 24, 2019

This issue is about the proposed removal of the eslintIntegration and tslintIntegration options.

On several projects, we use eslint 5.16.0, with which integration works successfully. And I was surprised when I opened the project an hour ago, but the extension did not work, and the linter integration settings were highlighted as unknown.

It seems to me that it is wrong to completely remove these properties before solving the problem with eslint 6. I suggest returning the properties and renaming them to something like oldEslintIntegration and oldTslintIntegration.

@ntotten
Copy link
Member

ntotten commented Aug 24, 2019

While I understand that the removal is an inconvenience. There were many more issues than just the ESLint 6 issue. When looking at usage (in public repos at least), it looks like usage of prettier-eslint is only about 10% of what eslint-prettier-config is. Given the limited resources on this project, my preference is to focus on the core use case - which is running prettier. There are a number of improvements I think we can make that I would prefer to spend time on rather than trying to keep ESLint working and dealing with all the issues.

As a temporary solution, you can install a specific version of the plugin https://code.visualstudio.com/updates/v1_30#_install-previous-versions

I'm happy to have the discussion, but when I solicited feedback on this a few weeks ago very few people responded. I am also happy to accept a pull request that adds back this integration - but I would prefer that if we add it back that we also detect and handle the cases of invalid configurations (which can be many) and better warn the users of why it isn't working and provide docs on how to fix it.

@ntotten ntotten changed the title Linters integration suggestion Linters integration discussion Aug 24, 2019
@AnechaS
Copy link

AnechaS commented Aug 24, 2019

Can you come back prettier.eslintIntegration

@ntotten
Copy link
Member

ntotten commented Aug 24, 2019

See my comment on #870. I am going to keep the existing behavior but mark it as deprecated. If the bugs are fixed in the downstream dependancies maybe we can keep it. If not, I will likely remove it in future versions.

@ntotten ntotten closed this as completed Aug 24, 2019
@ehaynes99
Copy link

Given that this is a vscode plugin and not a build tool, does the dependency graph of prettier-eslint actually tell you anything? I use this plugin instead of prettier and prettier-eslint in all of my projects.

I wonder if you would consider checking actual usage of the feature with this plugin. I haven't used it personally, but I think this would allow for that rather easily.
https://github.com/Microsoft/vscode-extension-telemetry

Here are some other metrics that might be noteworthy:

All that said, I haven't had time to learn enough about the inner workings of eslint to help with this problem, so I fully understand the time constraints. I also realize that a lot of the concerns users have with this are outside the scope of this plugin, but IMO "inconvenience" is an understatement. There is no less-convenient workaround within the editor. It completely breaks the functionality for anyone with that checkbox on...

@ntotten
Copy link
Member

ntotten commented Aug 24, 2019

@ehaynes99 Those stats aren't really deterministic or even related to this feature. This doesn't have anything to do with eslint-config-airbnb or eslint-config-standard. You can absolutely still use ESLint and prettier if we make this change. Please check out the documentation here. There are many ways to use prettier with eslint. I would say the way it is implemented with this integration is the least practical way to do so.

As mentioned in the above comment, I decided to not remove the linters for now, but rather mark them as deprecated. I am in the process of adding telemetry to determine usage. I suspect it is not that high, but I'll wait for the data.

@ehaynes99
Copy link

I'm not sure how that document -- which lists other ways to run prettier -- has to do with this plugin -- which runs prettier. I also don't see how it would be viewed as impractical to want a single editor command that formats code according to the project's formatting rules.

Anyway, I didn't want to start an argument, just my $0.02 for discussion. I've loved being able to use both tools with this plugin. If it's heading away from that, though, fair enough. Thanks for the deprecation strategy.

@ntotten
Copy link
Member

ntotten commented Aug 25, 2019

I do agree with you on there being some benefit about using a single tool and having simpler configuration, but I am also trying to weigh that against how much effort it is to support - and if it is really the best option. There have been a lot of changes to ESLint and VS Code over the last few years and in my opinion, you get the best setup by using both ESLint plugin and Prettier plugin with configs for both. Here is a good article on the type of setup I am recommending: https://dorshinar.me/linting-your-react+typescript-project-with-eslint-and-prettier

I push out a version this morning with telemetry to track usage of TSLint, ESLint, and Stylelint. So far almost nobody is using stylelint, about 2% are using TSLint, and about 10% are using ESLint. Given that TSLint is being deprecated in favor of ESLint and Stylelint gets almost no usage I think those are strong candidates for removal. ESLint obviously is harder to say. If those numbers hold for all users of this extension then 10% is a fair number. I will wait a bit and see if those numbers hold over time before I make the final call.

I would be interested in somebody who is passionate about the ESLint portion to help out with the project. If it is just me though, I am probably going to focus on other areas.

@ntotten ntotten reopened this Aug 25, 2019
@ntotten
Copy link
Member

ntotten commented Aug 26, 2019

Here are the stats so far. 13% average usage of ESLint integration. 3% for TSLint, and 2% for Stylelint.

image

@BPScott
Copy link
Member

BPScott commented Aug 26, 2019

First of all, thank you @ntotten for volunteering to help maintain prettier-vscode and your work so far :)

I'm happy to have the discussion, but when I solicited feedback on this a few weeks ago very few people responded

Sorry, I missed this the first time around :)

Context: I'm a core maintainer of stylelint-prettier and eslint-plugin-prettier, while I technically have a commit bit on prettier-eslint I've not interacted with that project. I'd be interested in helping out if it would ensure the continued existence of this feature.

Throwing a comment in here to back continuing to support prettier-eslint and prettier-stylelint.

My biggest concern is that by removing this functionality we push the complexity of having to configure multiple auto formatters onto the end user. As it currently stands it is straightforward to say "autoformat everything you can with the prettier plugin" and everything works. However removing this feature means two things happen:

  • The ability to autoformat style files using stylelint is lost entirely because the stylelint extension does not support autoformatting at all.
  • Users have to apply per-file extension configurations for their linting tools saying "format these files with eslint, these files with prettier, these files with stylelint", which while possible feels error prone and we would need to provide documentation on how that should be correctly setup.

By making the users configure this on their own there is scope for misconfiguration and
while it is faster to run plain eslint/stylelint in place of running prettier-eslint/prettier-stylelint (which under the hood runs prettier, then eslint/stylelint with prettier configured as a rule) the lost time isn't noticeable and helpfully avoids hard bugs where prettier's formatting can mess with other linting rules like prettier/eslint-plugin-prettier#65 (and this can't easily be fixed in eslint-plugin-prettier due to the tradeoffs mentioned in my final comment on that issue).

While having to maintain the integration with these liners is a burden, I think it is worth the effort for the improved developer experience it provides.


Tangentially related: How do those metrics work for cases where people have enabled the prettier plugin globally but have opted into the eslint/stylelint plugins on a per project basis? That is how the majority of our projects at work are configured. I worry that this might result in skewing the numbers to be lower than actual usage.

@ryanolsonx
Copy link

ryanolsonx commented Aug 27, 2019

I agree with @BPScott. That functionality removed was something a lot of folks have come to rely on.

Not cool.

@kylebolstad
Copy link

We have a team of developers who rely upon stylelint and Prettier in VSCode to auto-fix SCSS on save. At the time of this comment, I don't think there is any other way to do this in VSCode without prettier.stylelintIntegration set to true. The deprecation warning for prettier.stylelintIntegration is a bit confusing since it only mentions TSLint and ESLint. If there are other extensions that auto-fix SCSS on save in VSCode, we would consider switching.

@ntotten
Copy link
Member

ntotten commented Aug 30, 2019

@BPScott Hey, thanks for the comment. If you’re willing to help that certainly makes keeping it easier. If you have some time, I’d like to chat next week and see if we can come up with a good path forward to fixing these open issues and improving the experience to reduce all these bugs we seem to get.

Regarding the metrics, they are reported when a project is opened. The project config overrides the global, so the metrics should account for this.

@ntotten
Copy link
Member

ntotten commented Aug 30, 2019

@ryanolsonx nothing has been removed. That’s what this discussion is about.

@BPScott
Copy link
Member

BPScott commented Sep 3, 2019

Hi @ntotten, setting up a chat this week would be good. Drop me an email: ben.scott@shopify.com and we can sort out a date/time (I'm PST-based).

@ntotten
Copy link
Member

ntotten commented Sep 13, 2019

Moving the discussion to issue #958.

@ntotten ntotten closed this as completed Sep 13, 2019
@github-actions
Copy link

This issue has been automatically locked since there has not been any recent activity after it was closed. Please open a new issue for related bugs.

@github-actions github-actions bot added the locked Please open a new issue and fill out the template instead of commenting. label Apr 12, 2020
@github-actions github-actions bot locked as resolved and limited conversation to collaborators Apr 12, 2020
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
discussion locked Please open a new issue and fill out the template instead of commenting.
Projects
None yet
Development

No branches or pull requests

7 participants