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

linting.lintOnTextChange does not work + doc bug #28

Closed
stlaz opened this issue Nov 10, 2017 · 14 comments
Closed

linting.lintOnTextChange does not work + doc bug #28

stlaz opened this issue Nov 10, 2017 · 14 comments
Labels
area-linting bug Issue identified by VS Code Team member as probable bug

Comments

@stlaz
Copy link

stlaz commented Nov 10, 2017

Environment data

VS Code version: 1.18.0, 1.17.2
Python Extension version: 0.8.0
Python Version: 2.7.14
OS and version: Fedora 26 (4.13.11-200.fc26.x86_64), GNOME Shell 3.24.3

Actual behavior

Nothing happens after, e.g., exceeding a line

Expected behavior

The line exceeding 80 chars gets underlined after a while

Steps to reproduce:

  • Example: create a line with >80 characters

Logs

Output from Python output panel

Output from Console window (Help->Developer Tools menu)

file:///usr/share/code/resources/app/out/vs/workbench/workbench.main.js [Extension Host] (node:23105)
DeprecationWarning: os.tmpDir() is deprecated. Use os.tmpdir() instead.
raw.marked.js:15 [Violation] Added non-passive event listener to a scroll-blocking 'touchstart' event. Consider marking event handler as 'passive' to make the page more responsive.
raw.marked.js:15 [Violation] Added non-passive event listener to a scroll-blocking 'touchmove' event. Consider marking event handler as 'passive' to make the page more responsive.
raw.marked.js:15 [Violation] Added non-passive event listener to a scroll-blocking 'touchstart' event. Consider marking event handler as 'passive' to make the page more responsive.
raw.marked.js:15 [Violation] Added non-passive event listener to a scroll-blocking 'touchmove' event. Consider marking event handler as 'passive' to make the page more responsive.
raw.marked.js:15 [Violation] Added non-passive event listener to a scroll-blocking 'touchstart' event. Consider marking event handler as 'passive' to make the page more responsive.
raw.marked.js:15 [Violation] Added non-passive event listener to a scroll-blocking 'touchmove' event. Consider marking event handler as 'passive' to make the page more responsive.

Additional info:

Config:

    "python.linting.pep8Enabled": true,
    "python.linting.flake8Enabled": true,
    "python.linting.pylintEnabled": false,
    "python.linting.lintOnTextChange": true,
    "python.linting.lintOnSave": false

If I turn lintOnSave to true (which is the default, there's a bug in the docs - https://code.visualstudio.com/docs/python/linting), the issues in source code get properly marked on file save.

@DonJayamanne DonJayamanne added the bug Issue identified by VS Code Team member as probable bug label Nov 14, 2017
@DonJayamanne
Copy link

Agreed, the documentation needs to be updated and we'll look at removing the setting "python.linting.lintOnTextChange": true, (as this does not work as expected). Linting will only work off the contents of the file (and not what's in the editor buffer that isn't saved)

@stlaz
Copy link
Author

stlaz commented Nov 14, 2017

Does that mean you have to save the file every time to get its contents checked? Does VSCode not provide an interface to have the editor buffer checked? I believe I did see it for some other languages.

@DonJayamanne
Copy link

DonJayamanne commented Nov 14, 2017

Does that mean you have to save the file every time to get its contents checked

Yes

Does VSCode not provide an interface to have the editor buffer checked

It does, however this is a limitation of the extension.

@stlaz
Copy link
Author

stlaz commented Nov 14, 2017

Alright, thanks for the answers.

I suppose an RFE should be filed but it probably appears in the explosion of issues anyway, or may be related to some of them.

Feel free to close once the docs are fixed.

edit (27 Nov 2017): I see this is being referred to as to the actual RFE so please don't close unless implemented.

@DonJayamanne
Copy link

@brettcannon the setting python.linting.lintOnTextChange does not work as expected.

My suggestion is to remove this.
Currently for linting on text change to work, the changes have to be persisted to disc. I.e. linting won't work off the editor buffer.
The solution was to alway enable auto saving within vs code.

I.e. this was a poorly designed, hence needs to be removed to avoid any further confusion.
We'd need to update the documentation as well.

@TheRamsWay
Copy link

Continuously saving isn't really a solution

@brettcannon
Copy link
Member

@DonJayamanne yep, we should remove the setting and let people turn on auto-saving for now and long-term plan on LSP providing a more robust solution.

@DonJayamanne
Copy link

@brettcannon Agreed. I suggest we go with the approach we took with the deprecation of 'formatOnSave' setting.

  • Suggested message "The setting 'lintOnTextChange' is deprecated. Please use 'python.linting.lintOnSave' and 'files.autoSave'"
  • The 'Learn more' button would take the user to a github issue page (new Issue), describing this.
  • Why a new Github issue? Simple, this way we have all of the information nicely documented on the top. (also, this new issue will be closed).

What do you think?

@stlaz
Copy link
Author

stlaz commented Nov 28, 2017

Rather than claiming the option is deprecated, it might be better to say it's not implemented, remove all related code and point to the new issue. This both reflects the situation better and gives the users a feeling that you may be implementing it once.

@DonJayamanne
Copy link

DonJayamanne commented Nov 28, 2017

It should also be a painful-enough reminder to you that you need to implement such a basic functionality.

I don't think we need to have such messages in the extension to serve as reminder. That's where the github issue would come in

@stlaz
Copy link
Author

stlaz commented Nov 28, 2017

My point is - if you say it's deprecated, you're throwing the option away. If you just say it's not yet implemented, you can reuse it. I think lintOnTextChange is quite a good name for the functionality, let's not throw it away.

I do agree, however, that you should have the new issue as the RFE for this to be done. You should refer to it from this and https://github.com/DonJayamanne/pythonVSCode/issues/134 threads. You can see in both the places that there are people who care (although not enough to themselves create a PR 😢 - including me, sorry, no time 😞).

@brettcannon
Copy link
Member

@stlaz There's nothing preventing us from implementing the feature in the future and using the settings name again, but we are currently not prepared to promise implementing it in a time frame where we don't look bad for not getting to the feature in a timely manner while the setting sits around languishing.

@DonJayamanne
Copy link

Closing in favor of #313. If you want this feature, please create a new issue as an enhancement request. Thanks

@microsoft microsoft locked and limited conversation to collaborators Dec 12, 2017
@brettcannon
Copy link
Member

#408 is tracking the feature request

Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
area-linting bug Issue identified by VS Code Team member as probable bug
Projects
None yet
Development

No branches or pull requests

4 participants