Skip to content

Set is_saved to True in m_text_document__did_open#604

Merged
ccordoba12 merged 2 commits intopalantir:developfrom
casch-at:develop
Jun 24, 2019
Merged

Set is_saved to True in m_text_document__did_open#604
ccordoba12 merged 2 commits intopalantir:developfrom
casch-at:develop

Conversation

@casch-at
Copy link
Copy Markdown
Contributor

@casch-at casch-at commented Jun 18, 2019

Fixes #603.

@palantirtech
Copy link
Copy Markdown
Member

Thanks for your interest in palantir/python-language-server, @cslux! Before we can accept your pull request, you need to sign our contributor license agreement - just visit https://cla.palantir.com/ and follow the instructions. Once you sign, I'll automatically update this pull request.

@ccordoba12 ccordoba12 added this to the v0.27.0 milestone Jun 18, 2019
@ccordoba12 ccordoba12 changed the title Set is_saved to True in m_text_document__did_open (#603) Set is_saved to True in m_text_document__did_open Jun 18, 2019
@casch-at
Copy link
Copy Markdown
Contributor Author

casch-at commented Jun 18, 2019

Hmm, there is still something bogus I think.
When I open a file the m_text_document__did_open routine gets called, which sets is_saved to True, this method calls self.lint which is wrapped with _utils.py::debounce(), but somehow the value of is_saved gets changed to False :-/... inside the debounce function???

2019-06-18 20:05:49,205 UTC - ERROR - pyls.python_ls - did_open...is_saved=True
2019-06-18 20:05:49,205 UTC - ERROR - pyls._utils - call_args={'self': <pyls.python_ls.PythonLanguageServer object at 0x7f737f5e5f60>, 'doc_uri': 'file:///SOMEFILE.py', 'is_saved': True}
2019-06-18 20:05:49,210 UTC - ERROR - pyls._utils - call_args={'self': <pyls.python_ls.PythonLanguageServer object at 0x7f737f5e5f60>, 'doc_uri': 'file:///SOMEFILE.py', 'is_saved': False}
2019-06-18 20:05:49,711 UTC - ERROR - pyls.python_ls - lint...is_saved=False
2019-06-18 20:05:49,711 UTC - ERROR - pyls.plugins.pylint_lint - is_saved =False

Can someone enlighten me please, and tell me what's going on here.

Thanks!

EDIT:
After I call revert-buffer in Emacs the pylint_lint is correctly called with is_saved set to True

2019-06-18 20:17:01,376 UTC - ERROR - pyls.python_ls - did_open...is_saved=True
2019-06-18 20:17:01,377 UTC - ERROR - pyls._utils - call_args={'self': <pyls.python_ls.PythonLanguageServer object at 0x7f737f5e5f60>, 'doc_uri': 'file:///home/cschwarzgruber/Developing/python/magenta-wan-renewal/magenta_wan_renewal.py', 'is_saved': True}
2019-06-18 20:17:01,877 UTC - ERROR - pyls.python_ls - lint...is_saved=True
2019-06-18 20:17:01,878 UTC - ERROR - pyls.plugins.pylint_lint - is_saved =True

@ccordoba12
Copy link
Copy Markdown
Contributor

@cslux, so is this ready to be merged?

@mpanarin
Copy link
Copy Markdown
Contributor

Do not merge this. I tried this fix locally not so long ago with Emacs as well and experienced a bunch of issues, like error of "Wrong line number" or something like that. Had no time to debug them though.

@ccordoba12 ccordoba12 removed this from the v0.27.0 milestone Jun 19, 2019
@casch-at
Copy link
Copy Markdown
Contributor Author

I haven't seen this error. Can you try it again please?

@casch-at
Copy link
Copy Markdown
Contributor Author

@ccordoba12 Regarding the issue mentioned at #604 (comment). It seems to be indeed a lsp-mode.

When lsp-mode starts the python-language-server, the following things happen.

  • Sending request 'initialize
  • Received response 'initialize
  • Sending notification 'initialized'
  • Sending notification 'textDocument/didOpen'
  • Sending notification 'workspace/didChangeConfiguration
  • ...

Both didOpen and didChangeConfiguration requests will result in a self.lint in python_ls.py. With my change didOpen would call the pylint_lint with is_saved set to True where as didChangeConfiguration would call the pylint with is_saved set to False. But due to debouncing, didChangeConfiguration will win, and therefore pylint_lint will not run, but instead returns an empty diagnostics object.

When I open another file in the same project everything works as expected. No errors...

I think didChangeConfiguration should be called before didOpen.

@yyoncho
@vibhavp

@mpanarin
Copy link
Copy Markdown
Contributor

@ccordoba12 Regarding the issue mentioned at #604 (comment). It seems to be indeed a lsp-mode.

When lsp-mode starts the python-language-server, the following things happen.

  • Sending request 'initialize
  • Received response 'initialize
  • Sending notification 'initialized'
  • Sending notification 'textDocument/didOpen'
  • Sending notification 'workspace/didChangeConfiguration
  • ...

Both didOpen and didChangeConfiguration requests will result in a self.lint in python_ls.py. With my change didOpen would call the pylint_lint with is_saved set to True where as didChangeConfiguration would call the pylint with is_saved set to False. But due to debouncing, didChangeConfiguration will win, and therefore pylint_lint will not run, but instead returns an empty diagnostics object.

When I open another file in the same project everything works as expected. No errors...

I think didChangeConfiguration should be called before didOpen.

@yyoncho
@vibhavp

Okay, now i remember. The issue with line number i mentioned is when i added save=True to didChangeConfiguration so sorry for misleading info.

@casch-at
Copy link
Copy Markdown
Contributor Author

@mpanarin NP, thanks for checking again!

@gatesn
Copy link
Copy Markdown
Contributor

gatesn commented Jun 20, 2019

Shouldn't we just include the save kwarg in the key for debouncing?

@yyoncho
Copy link
Copy Markdown

yyoncho commented Jun 24, 2019

I think didChangeConfiguration should be called before didOpen.

I fixed that on lsp-mode side.

@casch-at
Copy link
Copy Markdown
Contributor Author

@yyoncho Thank you! Works now!

@ccordoba12 This can be merged now.

@ccordoba12
Copy link
Copy Markdown
Contributor

@gatesn, what do you say about this one?

@gatesn
Copy link
Copy Markdown
Contributor

gatesn commented Jun 24, 2019

The only odd thing is that is_saved might be presumed to mean that the current content of the file exists up-to-date on disk / can be accessed from the doc uri. But maybe it's not a problem

@ccordoba12 ccordoba12 added this to the 0.28.0 milestone Jun 24, 2019
@ccordoba12 ccordoba12 merged commit ded6489 into palantir:develop Jun 24, 2019
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.

pylint_lint.py returns existing diagnostics unless is_saved=True

6 participants