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.

Sign up for GitHub

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

Jump to bottom

PR: Migrate the Outline Explorer to use LSP information #13109

Merged
merged 33 commits into from
Sep 16, 2020
Merged

PR: Migrate the Outline Explorer to use LSP information #13109

merged 33 commits into from
Sep 16, 2020

Conversation

andfoy
Copy link
Member

@andfoy andfoy commented Jun 26, 2020 •
edited

Description of Changes

This PR migrates the Outline Explorer to use the information provided by the LSP via textDocument/documentSymbols call.

ezgif com-optimize

Loading spinner

Peek 04-09-2020 18-26

This PR depends on palantir/python-language-server#846

  • Wrote at least one-line docstrings (for any new functions)
  • Added unit test(s) covering the changes (if testable)
  • Included a screenshot or animation (if affecting the UI, see Licecap)

Issue(s) Resolved

Fixes #

Affirmation

By submitting this Pull Request or typing my (user)name below,
I affirm the Developer Certificate of Origin
with respect to all commits and content included in this PR,
and understand I am releasing the same under Spyder's MIT (Expat) license.

I certify the above statement is true and correct: @andfoy

@pep8speaks
Copy link

pep8speaks commented Jun 26, 2020 •
edited

Hello @andfoy! Thanks for updating this PR. We checked the lines you've touched for PEP 8 issues, and found:

Line 33:1: E302 expected 2 blank lines, found 1
Line 36:1: E302 expected 2 blank lines, found 1
Line 39:1: E302 expected 2 blank lines, found 1
Line 43:1: E302 expected 2 blank lines, found 1

Line 57:5: E265 block comment should start with '# '
Line 57:80: E501 line too long (80 > 79 characters)

Line 33:1: E302 expected 2 blank lines, found 1
Line 36:1: E302 expected 2 blank lines, found 1
Line 39:1: E302 expected 2 blank lines, found 1
Line 43:1: E302 expected 2 blank lines, found 1

Line 408:80: E501 line too long (80 > 79 characters)

Line 296:80: E501 line too long (108 > 79 characters)
Line 299:80: E501 line too long (106 > 79 characters)
Line 304:80: E501 line too long (108 > 79 characters)
Line 307:80: E501 line too long (109 > 79 characters)
Line 308:80: E501 line too long (109 > 79 characters)
Line 311:80: E501 line too long (109 > 79 characters)
Line 312:80: E501 line too long (109 > 79 characters)
Line 313:80: E501 line too long (109 > 79 characters)
Line 314:80: E501 line too long (109 > 79 characters)
Line 315:80: E501 line too long (108 > 79 characters)
Line 316:80: E501 line too long (108 > 79 characters)
Line 317:63: E231 missing whitespace after ':'
Line 317:80: E501 line too long (108 > 79 characters)
Line 318:80: E501 line too long (109 > 79 characters)
Line 319:80: E501 line too long (109 > 79 characters)
Line 320:63: E231 missing whitespace after ':'
Line 320:80: E501 line too long (108 > 79 characters)
Line 321:63: E231 missing whitespace after ':'
Line 321:80: E501 line too long (107 > 79 characters)
Line 322:63: E231 missing whitespace after ':'
Line 322:80: E501 line too long (107 > 79 characters)
Line 323:63: E231 missing whitespace after ':'
Line 323:80: E501 line too long (107 > 79 characters)

Comment last updated at 2020-09-16 20:53:48 UTC

@andfoy andfoy changed the base branch from master to 4.x June 26, 2020 02:26
@andfoy andfoy changed the title PR: Compute diff between symbols across codeeditor changes PR: Migrate the Outline Explorer to use LSP information Jun 26, 2020
@ccordoba12 ccordoba12 mentioned this pull request Jul 3, 2020
Closed
@andfoy andfoy marked this pull request as ready for review July 14, 2020 04:18
@ccordoba12 ccordoba12 added this to the Sprint July milestone Jul 14, 2020
@ccordoba12 ccordoba12 mentioned this pull request Jul 28, 2020
Closed
@ccordoba12
Copy link
Member

/show binder

@github-actions
Copy link

github-actions bot commented Aug 7, 2020

Binder 👈 Launch a Binder instance on this branch

@ccordoba12 ccordoba12 modified the milestones: Sprint July, v4.2.0 Aug 9, 2020
ccordoba12
ccordoba12 reviewed Aug 14, 2020
View reviewed changes
Copy link
Member

@ccordoba12 ccordoba12 left a comment

Choose a reason for hiding this comment

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

Thanks a lot for this @andfoy! I left an initial review for you to take a look at.

spyder/plugins/editor/widgets/tests/test_editor_and_outline.py Show resolved Hide resolved
spyder/plugins/editor/widgets/tests/test_editor_and_outline.py Show resolved Hide resolved
spyder/plugins/outlineexplorer/editor.py Outdated Show resolved Hide resolved
spyder/plugins/outlineexplorer/tests/test_widgets.py Show resolved Hide resolved
spyder/plugins/outlineexplorer/editor.py Outdated Show resolved Hide resolved
spyder/plugins/outlineexplorer/widgets.py Outdated Show resolved Hide resolved
spyder/plugins/outlineexplorer/widgets.py Show resolved Hide resolved
spyder/plugins/outlineexplorer/widgets.py Outdated Show resolved Hide resolved
spyder/utils/icon_manager.py Show resolved Hide resolved
spyder/utils/syntaxhighlighters.py Outdated Show resolved Hide resolved
@andfoy
Copy link
Member Author

andfoy commented Aug 14, 2020 •
edited

How does this work in VSCode?

It seems that variables are displayed, but imports not. Actually, most of the LSP servers I've tried this PR on, they all show variables as part of their response, not only the LSP

@andfoy
Copy link
Member Author

andfoy commented Aug 14, 2020

Methods and attributes of classes are displayed as variables and functions:

This is a limitation of the LSP spec, which does not consider attributes nor methods

@andfoy
Merge remote-tracking branch 'upstream/4.x' into outline_explorer_lsp
b109e06 
Merge!
@andfoy
Skip module definitions on Python
8864403
@andfoy
git subrepo clone --branch=fix_method_attribute_symbols --force https…
83b6c5b 
…://github.com/andfoy/python-language-server.git external-deps/python-language-server

subrepo:
  subdir:   "external-deps/python-language-server"
  merged:   "bb47a9058"
upstream:
  origin:   "https://github.com/andfoy/python-language-server.git"
  branch:   "fix_method_attribute_symbols"
  commit:   "bb47a9058"
git-subrepo:
  version:  "0.4.1"
  origin:   "https://github.com/ingydotnet/git-subrepo"
  commit:   "a04d8c2"
@andfoy
Remove os and sys
59d0d31
@andfoy
Merge remote-tracking branch 'upstream/4.x' into outline_explorer_lsp
bbfedb8 
Merge!
@andfoy
Add loading spinner and fix issues with multi-line class imports
b87ebf3
@andfoy
git subrepo clone --branch=fix_method_attribute_symbols --force https…
139a401 
…://github.com/andfoy/python-language-server.git external-deps/python-language-server

subrepo:
  subdir:   "external-deps/python-language-server"
  merged:   "b1d320b54"
upstream:
  origin:   "https://github.com/andfoy/python-language-server.git"
  branch:   "fix_method_attribute_symbols"
  commit:   "b1d320b54"
git-subrepo:
  version:  "0.4.1"
  origin:   "https://github.com/ingydotnet/git-subrepo"
  commit:   "a04d8c2"
@andfoy
git subrepo clone --branch=fix_method_attribute_symbols --force https…
0a8fb3e 
…://github.com/andfoy/python-language-server.git external-deps/python-language-server

subrepo:
  subdir:   "external-deps/python-language-server"
  merged:   "0b932ad77"
upstream:
  origin:   "https://github.com/andfoy/python-language-server.git"
  branch:   "fix_method_attribute_symbols"
  commit:   "0b932ad77"
git-subrepo:
  version:  "0.4.1"
  origin:   "https://github.com/ingydotnet/git-subrepo"
  commit:   "a04d8c2"
@andfoy
Remove commented sections
f064378
@andfoy
Merge remote-tracking branch 'upstream/4.x' into outline_explorer_lsp
141473f 
Merge!
@andfoy
Do not include imports as part of tests
08ef147
@ccordoba12
Copy link
Member

Thanks @andfoy for your latest updates to this PR! The spinner interaction is working great, but we're still showing a lot of unnecessary imports:

Selección_092

I think this is something that needs to be fixed because it constitutes an important regression with respect to what we have now:

Selección_093

andfoy and others added 5 commits September 11, 2020 20:06
@andfoy
Set include import symbols option to Fals
0faf0a6
@andfoy
git subrepo clone --branch=fix_method_attribute_symbols --force https…
da7ab18 
…://github.com/andfoy/python-language-server.git external-deps/python-language-server

subrepo:
  subdir:   "external-deps/python-language-server"
  merged:   "58bfa4599"
upstream:
  origin:   "https://github.com/andfoy/python-language-server.git"
  branch:   "fix_method_attribute_symbols"
  commit:   "58bfa4599"
git-subrepo:
  version:  "0.4.1"
  origin:   "https://github.com/ingydotnet/git-subrepo"
  commit:   "a04d8c2"
@andfoy
git subrepo clone --branch=fix_method_attribute_symbols --force https…
772425a 
…://github.com/andfoy/python-language-server.git external-deps/python-language-server

subrepo:
  subdir:   "external-deps/python-language-server"
  merged:   "a831e8c23"
upstream:
  origin:   "https://github.com/andfoy/python-language-server.git"
  branch:   "fix_method_attribute_symbols"
  commit:   "a831e8c23"
git-subrepo:
  version:  "0.4.1"
  origin:   "https://github.com/ingydotnet/git-subrepo"
  commit:   "a04d8c2"
@ccordoba12
git subrepo clone --branch=develop --force https://github.com/palanti…
b1ed973 
…r/python-language-server.git external-deps/python-language-server

subrepo:
  subdir:   "external-deps/python-language-server"
  merged:   "d81c7ba14"
upstream:
  origin:   "https://github.com/palantir/python-language-server.git"
  branch:   "develop"
  commit:   "d81c7ba14"
git-subrepo:
  version:  "0.4.1"
  origin:   "https://github.com/ingydotnet/git-subrepo"
  commit:   "a04d8c2"
@ccordoba12
Merge branch '4.x' into outline_explorer_lsp
a3258a3
ccordoba12
ccordoba12 approved these changes Sep 16, 2020
View reviewed changes
Copy link
Member

@ccordoba12 ccordoba12 left a comment

Choose a reason for hiding this comment

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

Thanks a lot @andfoy! This is a fantastic improvement!!

@ccordoba12 ccordoba12 merged commit ce9756a into spyder-ide:4.x Sep 16, 2020
ccordoba12 added a commit that referenced this pull request Sep 16, 2020
@ccordoba12
Merge from 4.x: PR #13109
1ba433e
@ccordoba12 ccordoba12 mentioned this pull request Sep 16, 2020
Merged
@andfoy andfoy deleted the outline_explorer_lsp branch September 16, 2020 23:15
@CAM-Gerlach
Copy link
Member

For the record, as requested by @ccordoba12 , some quick test results moving lines that result in Outline Explorer updates, all with default Spyder settings, on mainwindow.py starting from around line 1000 and moving for 100 lines:

Previous result without this PR on 4.x, Outline Explorer hidden: 13 lines/s
Current result with this PR, Outline Explorer hidden: 13 lines/s
Current result with this PR, Outline Explorer shown: 10 lines/s

The Outline Explorer appears to update approximately 1-2x second with perhaps 100-200 ms lag, which would explain most of the slowdown (assuming it was blocking or stealing cycles from GUI updates during most of that time).

This was referenced Sep 26, 2020
Closed
Merged
This was referenced Sep 30, 2020
Closed
Closed
Closed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@ccordoba12 ccordoba12 ccordoba12 approved these changes

Assignees

@andfoy andfoy

Labels
None yet
Projects
None yet
Milestone
v4.2.0
Development

Successfully merging this pull request may close these issues.

None yet
4 participants
@andfoy @pep8speaks @ccordoba12 @CAM-Gerlach