Show colliding step bindings when hovering an ambiguous step - #39
Merged
Conversation
DiagnosticsAggregator reported a hardcoded "Ambiguous step definition." message for every ambiguous step, even though MatchResult.CreateMultiMatch already builds a detailed error message listing every colliding binding (the same logic the legacy VS extension used). Since the diagnostic message is what both VS and VS Code show on hover, ambiguous steps only ever surfaced the generic one-liner instead of the matches causing the ambiguity. Use step.Result.GetErrorMessage() for the ambiguous diagnostic so hover lists the colliding step definitions, matching the legacy VS extension's behavior. Fixes #18 Co-Authored-By: Claude Sonnet 5 <noreply@anthropic.com>
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
1 participant
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Summary
Fixes #18: hovering an ambiguous step in both VS and VS Code now lists the step definitions that collide, instead of a generic "Ambiguous step definition." message.
Root cause
There's no custom hover handler in this repo — hover tooltips for step diagnostics come from the standard
textDocument/publishDiagnosticsmessage, which both editors render on hover by default.DiagnosticsAggregatorwas building the ambiguous diagnostic from a hardcoded constant ("Ambiguous step definition."), discarding the detailed message thatMatchResult.CreateMultiMatchalready assembles — a"Ambiguous steps:"header followed by every colliding binding'sToString()(step type, expression, and implementing method). That's the same logic the legacy Reqnroll.VisualStudio extension used to get its ambiguous-step tooltip, so this brings the new LSP-based server back to parity with it.Change
DiagnosticsAggregator.Aggregatenow usesstep.Result.GetErrorMessage()(falling back to the old generic message only if that's somehow null) when building the diagnostic for each ambiguous step.Test plan
dotnet test tests/LSP/Reqnroll.IdeSupport.LSP.Core.Tests— 429 passed, 1 skipped (pre-existing, unrelated)dotnet test tests/LSP/Reqnroll.IdeSupport.LSP.Server.Tests— 521 passedAmbiguous_step_produces_Error_diagnostic_with_correct_message_and_sourceto assert the message lists every colliding binding rather than pinning the old generic string🤖 Generated with Claude Code