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

Add option for project level diagnostics #323

Closed
wants to merge 12 commits into from

Conversation

i328638
Copy link

@i328638 i328638 commented Oct 25, 2017

No description provided.

@msftclas
Copy link

msftclas commented Oct 25, 2017

CLA assistant check
All CLA requirements met.

@dbaeumer
Copy link
Member

@i328638 thanks a lot for the PR. I looked at it and I think project level diagnostics would be cool for ESLint. However they way it is implemented will not scale very good since you read all files into memory (and this even happens sync) on server start.

The ESLint API offers to validate files using a path. It even offers wild card support. So if we implement such a feature it should make used of the ESLint API. In addition VS Code settings like files.exclude might be worthy to be honored.

@i328638
Copy link
Author

i328638 commented Nov 19, 2017

@dbaeumer ,
I have changed my implementation to use the ESlint Api method- executeOnFiles. please review.

Can you please give me some more information of the files.exclude settings?

@dbaeumer
Copy link
Member

dbaeumer commented Nov 21, 2017

I looked at the code and we should clearly separate project validation from single file navigation. For example the setting validateProject should not be a TextDocumentSettings since it is a global setting. And the triggering of a project validation shouldn't be faked as a TextDocument validation.

Here is how I as a user would expect project validation to work:

  • if enabled all file in my project should be validated
  • if I have 100s of file project validation should not bring my machine to a halt. The user still needs to be able to sufficiently type in VS Code.
  • the files I type in have preference over project validation. Errors in these files should not be delayed by a project validation run
  • project validation should re-run if configuration file or ESLint settings change. However this needs to be detected careful since project validation runs are expensive.

Can you check your implementation how it will behave in the above user scenarios.

The setting I was referring is files.exclude

});
validateMany(documents.all());
}
if(projectValidationUri && fsPath.indexOf(".eslintrc") !== -1) {
Copy link

Choose a reason for hiding this comment

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

maybe the file name should be exactly .eslintrc and not contain

@microsoft microsoft deleted a comment from msftclas Dec 13, 2017
@microsoft microsoft deleted a comment from msftclas Dec 13, 2017
@dbaeumer
Copy link
Member

In addition I noticed lately that ESLint does some cross file checking on imports. This complicates things a lot for proper project validation since we would need to build a dependency graph to proper solve this efficiently on typing.

@amiramw
Copy link

amiramw commented Feb 24, 2018

@dbaeumer does it mean that eslint LSP shouldn't be considered as a single file anymore but as a language with project system?

@dbaeumer
Copy link
Member

@amiramw yes that would be the consequence.

@feedthejim
Copy link

Hi, any updates on the PR?

@i328638
Copy link
Author

i328638 commented Aug 22, 2018

Hi,
I am interested to know if there are any updates regarding my recent pull request.
Could you please let me know if there are any changes I could implement to improve the code if needed.
In addition, I would like to know if there has been any progress with your implementation on this issue.

Thanks,
Naama

@dbaeumer
Copy link
Member

There are a couple of things that have changed which we need to take into consideration

  • the plugin contributes a task which basically validates the content of a workspace folder. This can be enabled via: "eslint.provideLintTask": true. The nice thing about the task is that it provides textual output as well which is available in the terminal. The downside is that it can never capture eslint fixes. So these two things look conflicting and I am not sure which I would prefer. Opinions ?
  • the eslint server is now multi workspace folder aware, however the PR only handles one project folder. Since the server receives the workspace folders it should use those as a project URI and validate those for which project validation is enabled (you basically have to get the configuration for the workspace folder)
  • I know that ESLint now also detects cross file issues. For example file A.js import B and B doesn't exist. If the user creates B a project validation should re-lint A to make the problem go away. But I have to say I never looked when ESLint now has API to determine that A as to be re-linted based on changes in B. @i328638 do you know this?

Base automatically changed from master to main March 2, 2021 14:45
@dbaeumer
Copy link
Member

dbaeumer commented Oct 8, 2021

I will close the PR since it doesn't have any traction. Please ping if you want to continue working on this.

@dbaeumer dbaeumer closed this Oct 8, 2021
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.

6 participants