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

Analysis engine should not analyze entire project #1861

Closed
DonJayamanne opened this issue Jun 4, 2018 · 17 comments · Fixed by #1977
Closed

Analysis engine should not analyze entire project #1861

DonJayamanne opened this issue Jun 4, 2018 · 17 comments · Fixed by #1977
Assignees
Labels
area-intellisense LSP-related functionality: auto-complete, docstrings, navigation, refactoring, etc. bug Issue identified by VS Code Team member as probable bug
Milestone

Comments

@DonJayamanne
Copy link

DonJayamanne commented Jun 4, 2018

I don't think the analysis engine should analyze the entire workspace folder and report syntax errors.
Currently that's the default behaviour:

Problems:

As a result we have a lot of noise and false positives being reported.
If the code base is quite large, then valid errors/messages returned by linters get buried in the noise.

I'm flagging this as a bug as it produces too many false positives, & would be too confusing for developers moving from Jedi to Analysis engine.

Solution:

  • Provide setting to turn on/off the ability to scan entire workspace for syntax errors.

@qubitron @brettcannon cc

@DonJayamanne DonJayamanne added bug Issue identified by VS Code Team member as probable bug area-intellisense LSP-related functionality: auto-complete, docstrings, navigation, refactoring, etc. needs PR labels Jun 4, 2018
@DonJayamanne DonJayamanne modified the milestone: May 2018 Jun 4, 2018
@MikhailArkhipov MikhailArkhipov self-assigned this Jun 5, 2018
@MikhailArkhipov
Copy link

There is already some filtering for virtual environments residing in the workspace. Basically we just need some logic to determine which folders must be excluded. It is simpler in VS that has 'white list' model in most projects.

@patrys
Copy link

patrys commented Jun 5, 2018

@MikhailArkhipov When I tested yesterday most errors reported were in fact from packages installed in my virtualenv (${workspaceRoot}/.direnv/python-3.6.5) so I don't think that filter works.

@brettcannon
Copy link
Member

Having a path component of site-packages is the best solution I can think of off the top of my head.

@patrys
Copy link

patrys commented Jun 5, 2018

I think the extensions is already aware of where the venv is? The other candidate would be node_modules as there are several Node packages using gyp as a build system.

@MikhailArkhipov
Copy link

PR is up microsoft/PTVS#4372

@DonJayamanne
Copy link
Author

  • Open PTVSD repo in VSC
  • Select Python 3 interpreter in VSC
  • 200+ of errors are reported in Problems Window (these are false positives as there is plenty of code written in PyDev that's Python 2 specific)

I'm still getting too many false positives.

Provide setting to turn on/off the ability to scan entire workspace for syntax errors.

@qubitron
Copy link

Should we disable full-workspace analysis by default to be consistent with current behavior? I haven't seen much ask for analyzing the entire project, mostly folks just want to see syntax errors are they're editing code.

@patrys
Copy link

patrys commented Jun 20, 2018

Full-workspace analysis is a great feature when it's not overzealous. Consider this comment as me asking for it to be available 😄

@qubitron
Copy link

Agreed it's useful! We would have the option available, we just need to decide what the right default is -- it depends on how many people will get overwhelmed with results like Don's.

@patrys
Copy link

patrys commented Jun 20, 2018

@qubitron It should be possible to add folders to the ignore list. I'm fine with spending some time up-front and I'm fine with "analyze the entire project" being off by default as long as it's a supported option.

@DonJayamanne
Copy link
Author

It should be possible to add folders to the ignore list

Solutions:

  • Sounds like a great candidate for a code action /c @MikhailArkhipov as well.
  • New setting with a list of folders/files to be excluded

@MikhailArkhipov
Copy link

MikhailArkhipov commented Jun 20, 2018

Ignore list is already supported. We pass

        const list: string[] = ['**/Lib/**', '**/site-packages/**'];
        this.getVsCodeExcludeSection('search.exclude', list);
        this.getVsCodeExcludeSection('files.exclude', list);
        this.getVsCodeExcludeSection('files.watcherExclude', list);
        this.getPythonExcludeSection('linting.ignorePatterns', list);
        this.getPythonExcludeSection('workspaceSymbols.exclusionPattern', list);

So you can add more patterns in either of those as needed.

As for new setting, or code actions and 'only report errors on active files' feature - please open separate issues so we can prioritize and track them appropriately.

I am going to close this one rather than recycling issues... ;-)

@DonJayamanne
Copy link
Author

this.getVsCodeExcludeSection('files.watcherExclude', list);

That's the solution. Thanks

@MikhailArkhipov
Copy link

@DonJayamanne - PTVSD repo shows no errors... Apart fro Pylint.

image

@DonJayamanne
Copy link
Author

I'm still getting the messages on the Mac with Python 3.6.5 - 64Bit.
What version of Python do you have selected? Most probably 2.7

screen shot 2018-06-20 at 12 00 50 pm

@MikhailArkhipov
Copy link

MikhailArkhipov commented Jun 20, 2018

Yes, but these are legal. You also get bunch for 'long' ints in 3.x. They are syntax errors for the selected interpreter.

image

@MikhailArkhipov
Copy link

Opened #2030

@lock lock bot locked as resolved and limited conversation to collaborators Jul 19, 2018
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
area-intellisense LSP-related functionality: auto-complete, docstrings, navigation, refactoring, etc. bug Issue identified by VS Code Team member as probable bug
Projects
None yet
Development

Successfully merging a pull request may close this issue.

5 participants