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

python variables pane: don't hide _ if it was changed by the user #6297

Merged
merged 5 commits into from
Feb 21, 2025

Conversation

austin3dickey
Copy link
Contributor

@austin3dickey austin3dickey commented Feb 11, 2025

Addresses #3366. This PR changes the Variables pane behavior in the Python extension so that it no longer hides the _ variable if it was explicitly changed by the user.

An example: type _ = 1 into the console. Before this change, _ wouldn't show up. Now it does.

Release Notes

New Features

  • N/A

Bug Fixes

QA Notes

Try messing around with the _ variable; it should work like other variables in the Variables pane. Examples:

_ = 1
_ = 2
_ = [1] * 1000
del _
from ibis import _

(the last one requires pip install ibis-framework)

@:console @:variables

Copy link

github-actions bot commented Feb 11, 2025

E2E Tests 🚀
This PR will run tests tagged with: @:critical @:console @:variables

readme  valid tags

@austin3dickey austin3dickey changed the title WIP: python: don't hide hidden variables that were changed by the user WIP: python Variables pane: don't hide hidden variables that were changed by the user Feb 11, 2025
@austin3dickey austin3dickey force-pushed the aus/underscores branch 2 times, most recently from 84fe67d to 4ae2174 Compare February 19, 2025 18:54
@austin3dickey austin3dickey changed the title WIP: python Variables pane: don't hide hidden variables that were changed by the user WIP: python Variables pane: don't hide _ if it was changed by the user Feb 19, 2025
value is different from the value in the hidden namespace.
"""
hidden = self.kernel.shell.user_ns_hidden or {}
if name == "_":
Copy link
Contributor Author

Choose a reason for hiding this comment

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

Originally I was planning to take this fork for any hidden variable. The hidden variable namespace includes (but isn't limited to) these variables:

hidden variables
_ih
_oh
_dh
In
Out
get_ipython
exit
quit
open
help
_exit_code
__pydevd_ret_val_dict
__warningregistry__
__nonzero__
__name__
__doc__
__package__
__loader__
__spec__
__builtin__
__builtins__
_
__
___
_i
_ii
_iii
_i1
_i2

However, we started poking around at that change and realized that some hidden variables don't play well.

  • _exit_code shows up when you run a %pip install magic that fails to install something, because at one point during that process, we start inspecting the variables and _exit_code happens to not match the hidden-namespace _exit_code. So this code thought the user had defined it.
  • __ was behaving strangely when I ran some tests that dealt with a complicated plot, for a similar reason.
  • More?? There are probably more edge cases.

I believe these problems would have been fixable with more time and effort spent. But fixing those problems seems super low-value, and wasn't in the original scope of the issue anyway. So I'm proposing this PR just fixes the behavior of _. We can always revisit if someone needs another variable to show up.

Copy link
Contributor

Choose a reason for hiding this comment

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

I don't see __ and _exit_code often (ever?) in the wild, but _ is pretty common. I agree this is the right level of fix for now!

@austin3dickey
Copy link
Contributor Author

Just adding some tests now.

@austin3dickey austin3dickey changed the title WIP: python Variables pane: don't hide _ if it was changed by the user python variables pane: don't hide _ if it was changed by the user Feb 20, 2025
@@ -75,7 +75,7 @@ def _prepare_shell(shell: PositronShell) -> None:
# manually add them to user_ns_hidden to replicate running in Positron.
shell.user_ns_hidden.update(
{
k: None
k: ""
Copy link
Contributor Author

@austin3dickey austin3dickey Feb 20, 2025

Choose a reason for hiding this comment

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

This is closer to what actually happens outside of tests (and fixes a test for me)

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I went through this file and added a test case for the variable name _ each time we do something with a variable. This was actually really valuable because it caught a lot of stuff.

Copy link
Contributor

Choose a reason for hiding this comment

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

It's useful to have varname parametrized as well instead of "x" hardcoded everywhere.

@austin3dickey austin3dickey marked this pull request as ready for review February 20, 2025 20:56

# ...except hidden variables, because %reset -s doesn't touch those
assert "_" in shell.user_ns
Copy link
Contributor Author

Choose a reason for hiding this comment

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

I learned that %reset -s (the soft reset) does not touch the underscore variable. So when we clear all variables, which calls this:

it doesn't remove the _ from the variables pane. I decided to leave that behavior as-is instead of messing with the reset command. So now the variables pane simply reflects what's happened, in that everything gets deleted except _, even when it's been overridden by the user.

Copy link
Contributor

@isabelizimm isabelizimm left a comment

Choose a reason for hiding this comment

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

works as anticipated! 🚀

]

# All user variables are removed
assert "x" not in shell.user_ns
assert "y" not in shell.user_ns

# ...except hidden variables, because %reset doesn't touch those
Copy link
Contributor

Choose a reason for hiding this comment

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

woah! TIL 🤯

Copy link
Contributor

@seeM seeM left a comment

Choose a reason for hiding this comment

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

Code changes LGTM, great to see the tests!

],
)
@pytest.mark.parametrize("varname", ["x", "_"])
Copy link
Contributor

Choose a reason for hiding this comment

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

Woah, I did not know that you can chain parametrize, very useful!

Copy link
Contributor Author

Choose a reason for hiding this comment

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

yeah at my last job I was guilty of writing a test with like 6 different parametrizations resulting in like 3500 cases 😂 coverage was really important!!

Copy link
Contributor

Choose a reason for hiding this comment

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

It's useful to have varname parametrized as well instead of "x" hardcoded everywhere.

@austin3dickey austin3dickey merged commit 1585bd0 into main Feb 21, 2025
28 checks passed
@austin3dickey austin3dickey deleted the aus/underscores branch February 21, 2025 15:09
@github-actions github-actions bot locked and limited conversation to collaborators Feb 21, 2025
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants