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

PR: Restore code cells and block comments in the Outline pane and fix other issues #13885

Merged
merged 32 commits into from
Oct 31, 2020

Conversation

andfoy
Copy link
Member

@andfoy andfoy commented Sep 30, 2020

Description of Changes

This PR introduces pyls-spyder as a dependency in order to restore code cells and block comments on the outline explorer. It also fixes some minor UI details such as idle spinners on files that do not have outline support and adding an option to hide variable symbols.

Code cells and block comments

imagen

Hide/display variable symbols

Peek 29-09-2020 22-55

  • 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 #13896
Fixes #13877
Fixes #13872

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

@andfoy andfoy added this to the v4.2.0 milestone Sep 30, 2020
@andfoy andfoy self-assigned this Sep 30, 2020
@andfoy
Copy link
Member Author

andfoy commented Sep 30, 2020

pyls-spyder feedstock PR on conda-forge: conda-forge/staged-recipes#12758

@pep8speaks
Copy link

pep8speaks commented Sep 30, 2020

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

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

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

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

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

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

Comment last updated at 2020-10-31 21:39:17 UTC

@andfoy
Copy link
Member Author

andfoy commented Sep 30, 2020

@jnsebgosselin, does this fixes the navigation issues?

Peek 30-09-2020 18-10

@jnsebgosselin
Copy link
Member

@jnsebgosselin, does this fixes the navigation issues?

I think you have not pushed the commits do do that yet?

@jnsebgosselin
Copy link
Member

jnsebgosselin commented Sep 30, 2020

Also, the option to hide the variables from the explorer does not seem to work for variables that are attributes of an object.

image

@andfoy
Copy link
Member Author

andfoy commented Sep 30, 2020

I think you have not pushed the commits do do that yet?

You're totally right!

@andfoy
Copy link
Member Author

andfoy commented Sep 30, 2020

Also, the option to hide the variables from the explorer does not seem to work for variables that are attributes of an object.

Since we are separating variables from attributes, I don't know if we want to hide attributes as well

@andfoy
Copy link
Member Author

andfoy commented Sep 30, 2020

@jnsebgosselin the commits are now in the branch

@jnsebgosselin
Copy link
Member

jnsebgosselin commented Oct 1, 2020

Also, the option to hide the variables from the explorer does not seem to work for variables that are attributes of an object.

Since we are separating variables from attributes, I don't know if we want to hide attributes as well

If this is possible, I would vote to hide the attributes as well. I think that for writing scripts, showing the variables and attributes can be very useful. However, when writing object oriented code, this add a lot of clutter to the explorer that is not that much useful. But maybe that's just me.

Edit: However, I think that disabling the auto-expand items to follow cursor will make this less of an issue and will not be that much of a problem for me after all.

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 for this! I left a (mostly cosmetic) review for you.

I still have to review how this one works locally.

requirements/conda.txt Outdated Show resolved Hide resolved
spyder/dependencies.py Outdated Show resolved Hide resolved
spyder/plugins/outlineexplorer/api.py Outdated 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 Outdated Show resolved Hide resolved
spyder/plugins/outlineexplorer/widgets.py Outdated Show resolved Hide resolved
spyder/plugins/outlineexplorer/widgets.py Outdated Show resolved Hide resolved
@jnsebgosselin
Copy link
Member

jnsebgosselin commented Oct 1, 2020

In the previous version, the outline explorer is reflecting the position of the cursor in the editor down to the innermost visible element in the tree, without expanding anything.

Currently however, the outline explorer is not updated at all when the position of the cursor changes and Always display the innermost element is unchecked. For example, in the screenshot below, I would expect the setup_filter to be highlighted in the outline explorer without being automatically expanded to highlight the root_path attribute.

image

@andfoy
Copy link
Member Author

andfoy commented Oct 1, 2020

@jnsebgosselin, does the latest commit improves the navigation experience when not navigating to the innermost element?

andfoy and others added 9 commits October 29, 2020 20:27
subrepo:
  subdir:   "external-deps/python-language-server"
  merged:   "1425f75b5"
upstream:
  origin:   "https://github.com/palantir/python-language-server.git"
  branch:   "develop"
  commit:   "1425f75b5"
git-subrepo:
  version:  "0.4.1"
  origin:   "https://github.com/ingydotnet/git-subrepo"
  commit:   "a04d8c2"
This avoids introducing new, unnecessary options for the LSP
Also remove unnecessary imports and variables
Also fix the "Display variables" option
…ptions

This way we can be sure that the tree is rebuilt after changing them
@ccordoba12 ccordoba12 changed the title PR: Restore code cells and block comments in the Outline pane and fix other minor issues PR: Restore code cells and block comments in the Outline pane and fix other issues Oct 31, 2020
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 for creating pyls-spyder and your awesome work here to complete the migration of the Outline to use the LSP architecture.

@ccordoba12 ccordoba12 merged commit 776c884 into spyder-ide:4.x Oct 31, 2020
ccordoba12 added a commit that referenced this pull request Oct 31, 2020
@andfoy andfoy deleted the re_enable_cells_blocks branch October 31, 2020 22:16
@ccordoba12
Copy link
Member

@spyder-ide/core-developers, @goanpeca, this PR added a new dependency to Spyder: pyls-spyder. You can install it with

conda install -c spyder-ide/label/dev pyls-spyder

@jnsebgosselin, please test the Outline next week with these changes to see if you can spot other regressions. Thanks for your help!

@jnsebgosselin
Copy link
Member

@jnsebgosselin, please test the Outline next week with these changes to see if you can spot other regressions. Thanks for your help!

I will do, thanks for the work that was done on this.

@jnsebgosselin
Copy link
Member

I cannot see code cells and block comments in the outline explorer even after this PR. Should I open an issue about it or you already know?

I haven't installed pyls-black yet though. Might this be related?

@ccordoba12
Copy link
Member

Did you install pyls-spyder?

@jnsebgosselin
Copy link
Member

jnsebgosselin commented Nov 5, 2020

I cloned the repo and added it to my PYTHONPATH and I'm running it from source.

@jnsebgosselin
Copy link
Member

jnsebgosselin commented Nov 5, 2020

mmmh I think spyder doesn't see plugins anymore if I add them like that. It used to work though, since that is how I have always installed spyder-notebook and now it seems this plugin doesn't show in spyder if I do this like that.

@ccordoba12
Copy link
Member

ccordoba12 commented Nov 5, 2020

Sorry but that's a Setuptools plugin (not a Spyder one), so it needs to be properly installed. Or at least you need to run pip install -e . in your clone.

@jnsebgosselin
Copy link
Member

Sorry but that's a Setuptools plugin (not a Spyder one), so it needs to be properly installed. Or at least you need to run pip install -e . on your clone.

Ok I understand. I will install it from conda and see if that works.

Is it the same reason why it is also happening with spyder-notebook too ? I can't simply run it from the git repo anymore?

@jnsebgosselin
Copy link
Member

Ok I confirm that it is working if I install pyls-spyder with conda, thank you very much!

@jitseniesen
Copy link
Member

Is it the same reason why it is also happening with spyder-notebook too ? I can't simply run it from the git repo anymore?

On my computer, spyder will find the spyder-notebook plugin if the plugin is in PYTHONPATH; you don't need to pip install it (though I prefer to do it that way). This is with Spyder 4.2.0.dev0 (Commit: e813165).

@CAM-Gerlach
Copy link
Member

CAM-Gerlach commented Nov 5, 2020

As a general note, pip install -e . is almost always preferable to manual PYTHONPATH hacking in the modern era (and directly obsoletes python setup.py install, python setup.py develop etc that I still see an unfortunate number of places), since it has the same desired effect as adding the repo to your path (changes to the repo will be reflected in the installed package), while also running the setup.py to install it properly, and allowing it to be accessed and managed with pip, conda, setuptools, pkg_resources, etc.

@jnsebgosselin
Copy link
Member

The feature looks even better than before since you removed the ----. I think this looks great, good job.

image

@jnsebgosselin
Copy link
Member

Is it the same reason why it is also happening with spyder-notebook too ? I can't simply run it from the git repo anymore?

On my computer, spyder will find the spyder-notebook plugin if the plugin is in PYTHONPATH; you don't need to pip install it (though I prefer to do it that way). This is with Spyder 4.2.0.dev0 (Commit: e813165).

Ok thank you very much for taking the time to answer.

As a general note, pip install -e . is almost always preferable to manual PYTHONPATH hacking in the modern era (and directly obsoletes python setup.py install, python setup.py develop etc that I still see an unfortunate number of places), since it has the same desired effect as adding the repo to your path (changes to the repo will be reflected in the installed package), while also running the setup.py to install it properly, and allowing it to be accessed and managed with pip, conda, setuptools, pkg_resources, etc.

I should start doing that, I agree, thanks for the tip.

But you know how it is... It all looks good on paper until it screw your python installation and you have to reinstall everything and I just don't have time for that at the moment so I'm not risking it lol.

@CAM-Gerlach
Copy link
Member

Yeah...if it works as is then yeah. In the future though manual PYTHON hacking and particularly python setup.py develop have a higher chance of breaking your environment than pip install -e . since the latter registers everything it does in a standardized fashion and performs a bunch of sanity checks, and they are making a number of improvements in that latter regard with all the UX and dependency validation work they are doing.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
6 participants