Magic command hover text plus some bug fixes#3604
Conversation
|
|
||
| result.Events.Should().NotContainErrors(); | ||
|
|
||
| requestInputWasSent.Should().BeFalse(); |
There was a problem hiding this comment.
consider: Add an AssertionScope since there are multiple asserts
There was a problem hiding this comment.
Typically that's only useful if the multiple asserts can fail independently. The first assert here is more of a sanity check.
|
|
||
| [Theory] | ||
| [MemberData(nameof(LanguageServiceCommands))] | ||
| public async Task Language_service_commands_do_not_trigger_input_requests(KernelCommand command) |
There was a problem hiding this comment.
If this test fails - will this display the name of the command that is failing? It would be nice if we could do something so that it is easier to know the command (and code) for the failure case
There was a problem hiding this comment.
Yes, test output shows the parameters for the failing case.
There was a problem hiding this comment.
Ah was asking if we can tell by looking at the view in the test runner itself without clicking to inspect the output. But not a huge issue either way.
There was a problem hiding this comment.
Yes, most test runners support this.
| hoverText = parameter.Description; | ||
| } | ||
|
|
||
| break; |
There was a problem hiding this comment.
Should we add a defualt: case to throw for unexpected node type?
There was a problem hiding this comment.
No. It's not an exceptional case. Some node types don't (currently) support hover text.
| var bindingResult = await bind(expressionNode); | ||
|
|
||
| if (bindingResult.IsSuccessful) | ||
| if (bindingResult is not null) |
There was a problem hiding this comment.
When can this return null? Should an error be reported (or an exception thrown) when null is returned?
There was a problem hiding this comment.
Maybe it can be modeled differently but we don't want to perform binding during language service commands, since the resulting directive command won't actually be invoked.
| if (expressionType is "input" or "password") | ||
| { | ||
| var parametersNode = expressionNode.ChildNodes.OfType<DirectiveExpressionParametersNode>().SingleOrDefault(); | ||
| if (command is SubmitCode) |
There was a problem hiding this comment.
Is this always going to be a SubmitCode? If yes, should this throw an exception if some other command is present?
There was a problem hiding this comment.
Submission splitting happens for many different command types, including language service commands. We don't want the non-SubmitCode commands to trigger user prompts though.
| bool allowByRef, | ||
| DirectiveExpressionNode expressionNode) | ||
| { | ||
| // FIX: (RequestSingleValueFromKernel) make sure this is not dispatched on language service commands |
There was a problem hiding this comment.
Did you intend to remove the // FIX comments before merge or is this something that needs to be fixed up in follow up PRs?
There was a problem hiding this comment.
I'll remove this one since it was resolved.
2 participants
This PR adds support for hover text in magic commands.
It also includes fixes for a couple of issues with the new magic command parser:
SubmitCode.#!importto--file#!import --fileparameter