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: Fix unchecking Display variables and attributes not updating tree (Outline explorer) #21481

Merged
merged 4 commits into from
Nov 3, 2023

Conversation

remisalmon
Copy link
Contributor

@remisalmon remisalmon commented Oct 31, 2023

Description of Changes

This fixes #21456 where unchecking "Display variables and attributes" in the outline does not work.

Issue debug log:

2023-10-31 11:28:00,930 [DEBUG] [spyder.plugins.editor.widgets.editor] -> Refresh EditorStack
2023-10-31 11:28:00,931 [DEBUG] [spyder.plugins.outlineexplorer.widgets] -> Set current editor to file /Users/remisalmon/test.py
2023-10-31 11:28:00,940 [DEBUG] [spyder.config.manager] -> Sending notification to observers of display_variables option in configuration section outline_explorer
2023-10-31 11:28:00,940 [DEBUG] [spyder.plugins.outlineexplorer.widgets] -> Current and new trees for file /Users/remisalmon/test.py are the same, so no update is necessary

I believe this uses the wrong operator from the intervaltree package where the difference operator does work as expected in this case:

with a file like:

def function():
    return

x = 0

we get, after unchecking Display variables and attributes:

current_tree=IntervalTree([Interval(0, 3, ((0, 2), function, 0b7cc8e0-5ff3-4535-b9e9-aa01e97ba571, False)), Interval(3, 4, ((3, 3), x, deb51838-ac1c-457a-b0ab-ce8449fd1e14, False))])
tree=IntervalTree([Interval(0, 3, ((0, 2), function, bbd41a82-41b7-460f-b56f-d5fca0f1e6f8, False))])
tree - current_tree = IntervalTree()
tree == current_tree = False

(the difference returns an empty tree object but the trees are different)

Another fix would be to look at changes = current_tree - tree but we do not need the actual difference tree object changes so this makes the code simpler.

This also fixes an issue where the tree would not update when "Display variables and attributes" is checked and a variable or attribute is removed from the code.

Tested locally.

Issue(s) Resolved

Fixes #21456

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: remisamon

@remisalmon remisalmon marked this pull request as ready for review October 31, 2023 18:20
@ccordoba12 ccordoba12 requested a review from dalthviz October 31, 2023 18:42
@ccordoba12 ccordoba12 added this to the v5.5.0 milestone Oct 31, 2023
@ccordoba12 ccordoba12 changed the title Fix unchecking display variables and attributes not updating tree in outlineexplorer PR: Fix unchecking Display variables and attributes not updating tree (Outline explorer) Oct 31, 2023
Copy link
Member

@dalthviz dalthviz left a comment

Choose a reason for hiding this comment

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

Thank you @remisalmon for helping checking for a fix! I think is okay to remove the tree being nonempty validation. Also, one missing thing is to add a test for this. Maybe the test could use the code example you used to illustrate the update issue, what do you think?

If you need help with the test let us know and again thank you for helping us with this!

spyder/plugins/outlineexplorer/widgets.py Outdated Show resolved Hide resolved
@pep8speaks
Copy link

pep8speaks commented Nov 2, 2023

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

Line 223:80: E501 line too long (86 > 79 characters)
Line 224:80: E501 line too long (87 > 79 characters)
Line 245:80: E501 line too long (80 > 79 characters)

Comment last updated at 2023-11-03 04:12:20 UTC

@remisalmon remisalmon force-pushed the fix-display-variables branch from 1428d32 to eff89d0 Compare November 2, 2023 04:46
@remisalmon
Copy link
Contributor Author

Ignore the pep8speaks message I force pushed a fix but it is not going away :)

I added a test that is passing locally (log below).

I could not figure out how to get qtbot to click on the DisplayVariables action that is not a button so I just forced it with toggle_variables() but maybe not ideal.

> py.test spyder/plugins/outlineexplorer/tests/test_widgets.py::test_display_variables
================================================= test session starts =================================================
platform linux -- Python 3.9.18, pytest-6.2.5, py-1.11.0, pluggy-1.3.0
PyQt5 5.15.9 -- Qt runtime 5.15.8 -- Qt compiled 5.15.8
rootdir: /home/remi/Work/GitHub/spyder, configfile: pytest.ini
plugins: flaky-3.7.0, cov-4.1.0, lazy-fixture-0.6.3, mock-3.12.0, order-1.0.1, qt-4.2.0, timeout-2.2.0
collected 1 item

spyder/plugins/outlineexplorer/tests/test_widgets.py .                                                          [100%]

================================================== warnings summary ===================================================
spyder/utils/programs.py:27
  /home/remi/Work/GitHub/spyder/spyder/utils/programs.py:27: DeprecationWarning: pkg_resources is deprecated as an API. See https://setuptools.pypa.io/en/latest/pkg_resources.html
    import pkg_resources

../../Miniforge3/envs/spyder-dev/lib/python3.9/site-packages/pkg_resources/__init__.py:2871
  /home/remi/Work/Miniforge3/envs/spyder-dev/lib/python3.9/site-packages/pkg_resources/__init__.py:2871: DeprecationWarning: Deprecated call to `pkg_resources.declare_namespace('sphinxcontrib')`.
  Implementing implicit namespace packages (as specified in PEP 420) is preferred to `pkg_resources.declare_namespace`. See https://setuptools.pypa.io/en/latest/references/keywords.html#keyword-namespace-packages
    declare_namespace(pkg)

../../Miniforge3/envs/spyder-dev/lib/python3.9/site-packages/jupyter_client/connect.py:22
  /home/remi/Work/Miniforge3/envs/spyder-dev/lib/python3.9/site-packages/jupyter_client/connect.py:22: DeprecationWarning: Jupyter is migrating its paths to use standard platformdirs
  given by the platformdirs library.  To remove this warning and
  see the appropriate new directories, set the environment variable
  `JUPYTER_PLATFORM_DIRS=1` and then run `jupyter --paths`.
  The use of platformdirs will be the default in `jupyter_core` v6
    from jupyter_core.paths import jupyter_data_dir, jupyter_runtime_dir, secure_write

-- Docs: https://docs.pytest.org/en/stable/warnings.html
===Flaky Test Report===

test_display_variables passed 1 out of the required 1 times. Success!

===End Flaky Test Report===
============================================ 1 passed, 3 warnings in 1.34s ============================================

@remisalmon remisalmon requested a review from dalthviz November 2, 2023 04:56
@dalthviz
Copy link
Member

dalthviz commented Nov 2, 2023

Awesome @remisalmon ! I think is okay to use the toggle_variables directly and don't worry about the pep8 message (usually we don't enforce them if they are only complaining about tests files). Could you remove the TODOs regarding the usage of qtbot please? Other than that this LGTM 👍

@remisalmon
Copy link
Contributor Author

Should be all good now, thank you @dalthviz for reviewing this.

Copy link
Member

@dalthviz dalthviz left a comment

Choose a reason for hiding this comment

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

Thanks @remisalmon !

@dalthviz dalthviz merged commit f487984 into spyder-ide:5.x Nov 3, 2023
dalthviz added a commit that referenced this pull request Nov 3, 2023
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants