microsoft / ptvsd

Python debugger package for use with Visual Studio and Visual Studio Code.
Other
550 stars 68 forks source link

Put dunder global variables in their own scope #1621

Closed karthiknadig closed 4 years ago

karthiknadig commented 5 years ago

Ideally the following should go into their own group.

fabioz commented 4 years ago

Note: I'm taking a look at this.

fabioz commented 4 years ago

Also related: https://github.com/microsoft/ptvsd/issues/763

fabioz commented 4 years ago

Also related: https://github.com/microsoft/ptvsd/issues/118

fabioz commented 4 years ago

@karthiknadig @int19h I'm still working this issue (along with the other related issues I commented above) -- I'm in the process of updating test cases now and checking for corner cases.

I separated entries in 4 groups (dunder variables, protected variables, function variables, class variables) -- anything that doesn't enter one of those groups appears directly below the variable (note that a variable goes into the group it matches first there even if it could appear at more than one group).

So, I'd like to gather comments on whether you think that's ok or if you think something should be changed in that organization...

The current result is in the screenshot below.

image

int19h commented 4 years ago

It looks great. We might want to fiddle with what exactly goes into which category if there's conflicts, but that can be a separate enhancement.

@brettcannon Is there some better name than "dunder variables" for that category?

int19h commented 4 years ago

One other thing we'll need is a boolean property in debug configuration to enable/disable grouping, something like "groupVariables". This will allow vscode-python to gradually roll it out via the experiment framework, and for users to explicitly opt out afterwards.

brettcannon commented 4 years ago

The ones in the example are technically global variables. They are also all standard module attributes. So it really depends on what you're trying to filter out: things that just happen to be named with a leading and trailing __ or specifically module attributes that exist on all/most modules?

int19h commented 4 years ago

I think technically we want to filter out the "magic" variables. It doesn't have anything to do with whether it's globals or not - as you can see on the screenshot, self also has that category.

fabioz commented 4 years ago

@brettcannon

For me it's a way to show variables that are seldomly used (and this includes both heuristics based on the name and variable type) without cluttering the UI too much.

As a bit of history, until now the debugger was always filtering out all of those (so, they'd only be accessible by having the user evaluate them on the cases he actually needed them) -- you can see the actual logic in https://github.com/microsoft/debugpy/blob/0dd33dd250a884bfa2b4f5025b15d2a29575646c/src/debugpy/_vendored/pydevd/_pydevd_bundle/pydevd_resolver.py#L187 -- with the proposed changes, the user would be able to access those through a group that can be expanded.

@int19h When that setting is turned on, those variables would disappear (as was the case until now) or would they show along all other variables?

int19h commented 4 years ago

I was thinking of the setting that covers this whole functionality (i.e. also protected, function etc). But we might want to think about something more fine-grained than that - e.g. some users might not like separating functions into a different category, and some might not like protected, but will still want dunder. So, maybe something like:

"groupVariables": ["function", "class", "dunder"]

It's unfortunate that we have to do this on the debugger side. Really, what it should be doing is just reporting variables tagged with categories, and the client should then show them in groups (or not) according to their configuration. Users shouldn't have to edit launch.json for this, because it's not a session-specific preference, but user-specific. Which means that the extension will have to surface it in user settings, and then inject it into debug configuration, much like we do with "pythonPath".

So, basically like DAP scopes, but applicable on every level of the hierarchy. This needs DAP and client support though.

int19h commented 4 years ago

Thinking about this more - since we'd want to also accommodate the existing behavior of hiding dunder entirely, how about:

"variablePresentation": {
    "function": "show",
    "dunder": "hide",
    "class": "group",
    ...
}
brettcannon commented 4 years ago

So basically you're trying to group Python-defined or Python-created variables? Maybe something along those names if you're going to include self?

int19h commented 4 years ago

The "dunder" group does not include self. What I was saying is that the screenshot shows a "dunder" group under self, so it's not just for predefined module variables. It's really any variable with __ around, but the intended purpose of this is to show "magic" ones separate from regular ones.

brettcannon commented 4 years ago

Technically they are special methods, informally called magic methods, and are typically pronounced "dunder ".

brettcannon commented 4 years ago

So take your pick? 😉 Or ask Luciana or someone who might not have as much exposure to the concept to see which name makes the most sense. (My guess is not "dunder" since you won't know that pronunciation without hearing someone say it and it typically isn't written down.)

karthiknadig commented 4 years ago

/cc @luabud

fabioz commented 4 years ago

I'll rename dunder variables to special variables as it seems better for me -- and I like the idea of configuring it with:

"variablePresentation": {
    "function": "show",
    "dunder": "hide",
    "class": "group",
    ...
}

If someone has a different opinion, please let me know ;)

luabud commented 4 years ago

I really like Pavel's suggestion and I also prefer special over dunder. My only question is regarding the difference between "show" and "group". Would it be that those who have "show" in it would be displayed all together without a parent?

int19h commented 4 years ago

Yep, that's the idea. Basically, the current behavior, just for that particular category.

Maybe it's not the best pair of terms to denote the difference, since "group" also shows the item, just in a different way. I don't know what the good concise term for "show without grouping" would be - "inline", maybe? We need native English speakers :)

karthiknadig commented 4 years ago

/cc @rchiodo We are using this mechanism as a way to reduce what we show in the variables window.

fabioz commented 4 years ago

Ok, I've provided a pull request where the options are group, inline, hide.

The default is:

        "variablePresentation": {
            "special": "group",
            "function": "group",
            "class_": "group",
            "protected": "inline",
        },

It's also possible to specify an entry with all: <some option> which sets the default value for all entries -- that way it's possible to do something as:

        "variablePresentation": {
            "all": "hide",
            "protected": "inline",
        },

I plan on merging later today.

adnan333bd commented 3 years ago

Thinking about this more - since we'd want to also accommodate the existing behavior of hiding dunder entirely, how about:

"variablePresentation": {
    "function": "show",
    "dunder": "hide",
    "class": "group",
    ...
}

where this configuration goes to, launch.json ?

int19h commented 3 years ago

@adnan333bd Yep, it should be in your launch.json if using VSCode.

Note that the category names changed slightly in the later comments - you're using "dunder", but it's "special" now.