Skip to content

Move off of options for encoding ephemeral state in inline-hints#67367

Merged
CyrusNajmabadi merged 5 commits intodotnet:mainfrom
CyrusNajmabadi:inlineHints
Mar 20, 2023
Merged

Move off of options for encoding ephemeral state in inline-hints#67367
CyrusNajmabadi merged 5 commits intodotnet:mainfrom
CyrusNajmabadi:inlineHints

Conversation

@CyrusNajmabadi
Copy link
Copy Markdown
Contributor

Fixes #57283

@CyrusNajmabadi CyrusNajmabadi requested review from akhera99 and tmat March 19, 2023 19:01
@CyrusNajmabadi CyrusNajmabadi requested a review from a team as a code owner March 19, 2023 19:01
@CyrusNajmabadi CyrusNajmabadi requested a review from a team March 19, 2023 19:01
@ghost ghost added the Area-IDE label Mar 19, 2023
{
Task<ImmutableArray<InlineHint>> GetInlineHintsAsync(Document document, TextSpan textSpan, InlineHintsOptions options, CancellationToken cancellationToken);
Task<ImmutableArray<InlineHint>> GetInlineHintsAsync(
Document document, TextSpan textSpan, InlineHintsOptions options, bool displayAllOverride, CancellationToken cancellationToken);
Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@tmat i could also make this part of InlineHintsOptions. It just wouldn't be populated from any options-service, but would be provided directly by the feature. However, i wasn't certain if that was a pattern we wanted for our immutable-options/options-services in the future. So, for now, i pulled out 'displayAllOverride' as a separate explicit option that is passed along.

If it is fine to move into InlineHintsOptions, i'm happy to move into it. BUt i wanted to check with you.

var hints = await service.GetInlineHintsAsync(
document, snapshotSpan.Span.ToTextSpan(), options,
displayAllOverride: _inlineHintKeyProcessor?.State is true,
cancellationToken).ConfigureAwait(false);
Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@tmat instead of this, we woudl do a with on the value returned by GlobalOptions.GetInlineHintsOptions(document.Project.Language); and would supply this value there.

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Either way is fine. Whatever feels better for the service API.

@CyrusNajmabadi
Copy link
Copy Markdown
Contributor Author

/azp run

@azure-pipelines
Copy link
Copy Markdown

Azure Pipelines successfully started running 4 pipeline(s).

@CyrusNajmabadi
Copy link
Copy Markdown
Contributor Author

/azp run

@azure-pipelines
Copy link
Copy Markdown

Azure Pipelines successfully started running 4 pipeline(s).

Copy link
Copy Markdown
Member

@333fred 333fred left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

O# change looks fine to me.

@CyrusNajmabadi CyrusNajmabadi merged commit 1a73331 into dotnet:main Mar 20, 2023
@ghost ghost added this to the Next milestone Mar 20, 2023
@CyrusNajmabadi CyrusNajmabadi deleted the inlineHints branch March 20, 2023 18:14
@allisonchou allisonchou modified the milestones: Next, 17.6 P3 Mar 28, 2023
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

Projects

None yet

Development

Successfully merging this pull request may close these issues.

Remove InlineHintsOptions.DisplayAllOverride

5 participants