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

dap: auto-load partially loaded nested variables #2453

Closed
wants to merge 1 commit into from

Conversation

hyangah
Copy link
Contributor

@hyangah hyangah commented Apr 26, 2021

Keep track of whether a variable is partially loaded in the variable
handle. When a partial variable is requested (since user manually
expands the variable through UI or debug console), try to load with
the expression, and replace the partially loaded previous variable
with the newly updated one.

Unfortunately, the newly loaded variable may be still partial
if the struct has many fields, or the variable is a map, array, or
struct. They will be addressed separately - probably by utilizing
pagination UI.

Keep track of the goid and frame information, so they are
available when the partially loaded variable is auto-loaded.

Before:
Screen Shot 2021-04-26 at 11 25 43 AM

After:
Screen Shot 2021-04-26 at 11 25 06 AM

Keep track of whether a variable is partially loaded in the variable
handle. When a partial variable is requested (since user manually
expands the variable through UI or debug console), try to load with
the expression, and replace the partially loaded previous variable
with the newly updated one.

Unfortunately, the newly loaded variable may be still partial
if the struct has many fields, or the variable is a map, array, or
struct. They will be addressed separately - probably by utilizing
pagination UI.

Keep track of the goid and frame information, so they are
available when the partially loaded variable is auto-loaded.
@hyangah
Copy link
Contributor Author

hyangah commented Apr 26, 2021

cc @polinasok @suzmue

Again - this is a low priority, non-v0 feature. So, review isn't urgent.

And, the TeamCity build failure looks like a build system failure.

mingw32-make : The term 'mingw32-make' is not recognized as the name of a cmdlet, function, script file, or operable
program. Check the spelling of the name, or if a path was included, verify that the path is correct and try again.
  At Z:\buildAgent\work\3aca8f960d346a9c\_scripts\test_windows.ps1:69 char:1
  + mingw32-make test
      + CategoryInfo          : ObjectNotFound: (mingw32-make:String) [], CommandNotFoundException
      + FullyQualifiedErrorId : CommandNotFoundException
      ```

@polinasok
Copy link
Collaborator

polinasok commented Apr 27, 2021

Hmm, I think we got our wires crossed somewhere because I see auto-loading as my deliverable on our quarterly plan. It is marked as in-progress because I have been already working on it. I took a quick look at your version, and you are using a different approach, which I think is more complicated at the first glance and has a couple of bugs, but I need to take a closer look to confirm. It looks like it keeps unnecessarily reloading partial variables like maps or arrays. I think it looses the not-loaded label and still presents an option to expand in cases where auto-loading failed (rare, but possible). It also doesn't refresh the inlined parent value. Tracking goroutine and frame id should not be necessary with frame-independent expressions.

My solution (which was already in place for interfaces in the code) has those issues addressed. I was going to just reuse it for everything else, plus additional testing and ironing out some corner cases, but was not done with the change because, like you said, it was not high priority. Here is PR with my version so far: #2455. I turned the things that I haven't finished into TODOs/comments.

@hyangah
Copy link
Contributor Author

hyangah commented Apr 27, 2021

Yes, this PR didn't intend to address the map, etc. I will close this PR and wait for the other PR.

@hyangah hyangah closed this Apr 27, 2021
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants