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

Separate REPL evaluation from it's result; fixes #79196 #80422

Merged
merged 1 commit into from Sep 10, 2019

Conversation

@dgozman
Copy link
Contributor

commented Sep 5, 2019

REPL now has two elements: one for the evaluation text added immediately (EvaluationReplElement), and another for the evaluation result once available (Expression).

@dgozman

This comment has been minimized.

Copy link
Contributor Author

commented Sep 5, 2019

@isidorn Please take a look. I went with two separate objects strategy, as you were suggesting. Thank you!

@isidorn isidorn added this to the September 2019 milestone Sep 6, 2019
@isidorn

This comment has been minimized.

Copy link
Contributor

commented Sep 6, 2019

@dgozman nice work, I really like it.
However I would prefer if we clean this up a bit. For example I feel like we are abusing the Expression class here since we only use the value. And we introduced an issue since the toString is now not correct in the Watch Expression view since the Expression is also used there.

Due to that I suggest the following:

  1. Move all the REPL specific classes from debugModel.ts to replModel.ts (SimpleReplElement, EvaluationReplElement, RawObjectReplElement)
  2. Instead of reusing the Expression class I suggest we introduce a new class which will live in replModel.ts and extend ExpressionContainer and will only be used in repl. It will not have the unneded stuff from Expression, and the expression.evaluate can be moved to a helper method that both of these can use
  3. We get rid of stale css class names output.expression both in repl.css and repl.ts and we use our new names.

This new class and the EvaluationReplElement are pairs. Due to that we should give them nice name. Here are some ideas and I am open to suggestions:

  • ReplEvaluationInput and ReplEvaluationOutput
  • ReplEvaluationExpression and ReplEvaluationResult
  • ReplEvaluationInput and ReplEvaluationResult (might be my favorite combination)

Let me know what you think. Thanks a lot!

@dgozman dgozman force-pushed the dgozman:fix-79196 branch from 2f34d3f to f2fbdb3 Sep 7, 2019
@dgozman

This comment has been minimized.

Copy link
Contributor Author

commented Sep 7, 2019

@isidorn Thank you for detailed feedback! I followed your suggestions, please take another look.

  • Went with ReplEvaluationInput and ReplEvaluationResult.
  • Moved code to replModel.ts.
  • Removed .input.expression, .output.expression and .input-output-pair from repl evaluations. We still need .expression, to pickup styling from renderExpressionValue. Also, there are still .output.expression left in other repl elements, which I'd like to not touch here. Is that ok?
}
}

export class ReplEvaluationResult extends ExpressionContainer implements IReplElement, IExpression {

This comment has been minimized.

Copy link
@isidorn

isidorn Sep 9, 2019

Contributor

I do not think ReplEvaluationResult should implement IExpression and it should not have a public name which we do not use anywhere.
I see that you are passing ReplEvaluationResult to renderExpressionValue, but can we pass just it's value instead.
I think we get rid of the name it will get clearer that the ReplEvaluationResult is just the result

I might be missing something here, but idealy we can simplify this.

This comment has been minimized.

Copy link
@dgozman

dgozman Sep 9, 2019

Author Contributor

Unfortunately, just passing value to renderExpressionValue does not work, since it expects some specific classes and properties.

I lifted value, type and valueChanged from IExpression to IExpressionContainer, since these are actually implemented by ExpressionContainer. This way only IExpression has a name, but we reuse value handling and make renderExpressionValue take IExpressionContainer. What do you think?

src/vs/workbench/contrib/debug/common/replModel.ts Outdated Show resolved Hide resolved
src/vs/workbench/contrib/debug/browser/repl.ts Outdated Show resolved Hide resolved
@isidorn

This comment has been minimized.

Copy link
Contributor

commented Sep 9, 2019

This looks great, thanks a lot!
I have left some comments inline in the code. Can you please tackle those and then we can merge this in? Apart from that:

  • Can you please merge with latest from master and resolve conflicts? There is one simple conflict in debugSession.ts
  • You did changes in the getHeight method and they make sense, however can you please test that the REPL scroll position behaves as expected when new output comes. Here's an example of a bug #71737 which we can potentially break by changing the getHeight
  • Did you run the smoke test so we verify we did not break anything with the css names? Here's how to run it https://github.com/Microsoft/vscode/blob/master/test/smoke/README.md
  • Is it possible to write a unit test that captures these new behavior so we are sure we do not break it in the future? Adding an element immediatly adds it to the repl, and once it is evaluated we check there is an additional element?
  • Not touching .output.expression makes sense, that is not directly related to this PR.

Thanks again!

REPL now has two elements: one for the evaluation text added immediately (ReplEvaluationInput),
and another for the evaluation result once available (ReplEvaluationResult).
@dgozman dgozman force-pushed the dgozman:fix-79196 branch from f2fbdb3 to 3bcaff8 Sep 9, 2019
Copy link
Contributor Author

left a comment

Thank you for thorough review, @isidorn. I made the changes, please take another look.

  • You did changes in the getHeight method and they make sense, however can you please test that the REPL scroll position behaves as expected when new output comes. Here's an example of a bug #71737 which we can potentially break by changing the getHeight

I played with it, and I cannot reproduce #71737 with this PR. That said, when repl scroll is not at the very bottom, it does sometimes jump by 1-2 pixels when new output arrives. I can repro the same without this PR though.

Yep, see the changes in debugSmoke.ts.

  • Is it possible to write a unit test that captures these new behavior so we are sure we do not break it in the future? Adding an element immediatly adds it to the repl, and once it is evaluated we check there is an additional element?

Perhaps I am missing something, but I didn't find a unit test which does have an actual DebugSession to test the whole pipeline. The ReplEvaluationInput being added immediately is already covered in debugModel.test.ts. Any pointers are welcome.

src/vs/workbench/contrib/debug/browser/repl.ts Outdated Show resolved Hide resolved
}
}

export class ReplEvaluationResult extends ExpressionContainer implements IReplElement, IExpression {

This comment has been minimized.

Copy link
@dgozman

dgozman Sep 9, 2019

Author Contributor

Unfortunately, just passing value to renderExpressionValue does not work, since it expects some specific classes and properties.

I lifted value, type and valueChanged from IExpression to IExpressionContainer, since these are actually implemented by ExpressionContainer. This way only IExpression has a name, but we reuse value handling and make renderExpressionValue take IExpressionContainer. What do you think?

src/vs/workbench/contrib/debug/common/replModel.ts Outdated Show resolved Hide resolved
@isidorn

This comment has been minimized.

Copy link
Contributor

commented Sep 10, 2019

Great work, I like that we moved those attributes up to IExpressionContainer.
As for the unit test to test the whole debug session flow - we do not have. Since that would require a real connection to the adapter, and spawing processes which felt liek out of scope for unit tests. Though we should probably invest in writing some integration tests for debugging.

I also tried out your changes and tehy look awesome to me. Merging in. Thanks!

@isidorn isidorn merged commit 4e3b4b6 into microsoft:master Sep 10, 2019
2 checks passed
2 checks passed
VS Code #20190909.55 succeeded
Details
license/cla All CLA requirements met.
Details
@isidorn isidorn added the debug label Sep 10, 2019
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
2 participants
You can’t perform that action at this time.