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

Cohost document highlighting #10656

Merged
merged 10 commits into from
Jul 23, 2024

Conversation

davidwengier
Copy link
Contributor

Part of #9519

Brings document highlight to cohosting, including tests. Also added some basic tests for RazorServices and Services.props.

@davidwengier davidwengier requested review from a team as code owners July 20, 2024 04:24
@davidwengier davidwengier removed the request for review from a team July 20, 2024 04:25
@davidwengier davidwengier changed the title Create endpoint and remote service interface Cohost document highlighting Jul 20, 2024
Copy link
Member

@DustinCampbell DustinCampbell left a comment

Choose a reason for hiding this comment

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

Looks good!

[DataContract]
internal record struct RemoteResponse<T>(
[property: DataMember(Order = 0)] bool StopHandling,
[property: DataMember(Order = 1)] T Result)
Copy link
Member

Choose a reason for hiding this comment

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

I love elevating this concept so that any service can use it. I have a few ideas and thoughts that you can take or leave:

  1. Consider whether Data or something else is a better name. To me, the words Result and Response are bit too close semantically and are pretty much interchangeable.
  2. Is it possible to add a T : class constraint?
  3. It might be helpful to add HasData and/or TryGetData(...) APIs. If T is constrained to class, declare Data as T? and then add [MemberNotNullWhen(true, Data)] to HasData and [NotNullWhen(true)] to TryGetData.
  4. StopHandling feels a little awkward to "handle" on the client side. I wonder if there's a better name or combination of signals. In general, it feels strange that the remote service would have so much knowledge about what the client does with the response. It also feels a bit odd to me that a RemoteResponse might have a Result and StopHandling == false. I don't have any great suggestions, so please don't make any changes related to this unless you have better ideas. However, I suggest that we keep thinking on this logic.

Copy link
Contributor Author

@davidwengier davidwengier Jul 22, 2024

Choose a reason for hiding this comment

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

This type is the epitomy of "naming is hard". Trying to work out names for the parameters, and the helper properties, that didn't clash and meant something etc. I do not disagree with any of your feedback.

I'd like to re-use part of this PR in the inlay hint work I'm currently doing though, so if you don't mind I think I'll merge this as is, but will do another pass on this class specifically in a separate PR. I think making it not a record might be the key, so we can separate out consructor params, properties etc. and allow some more nuance in these.

I will say for your item number 2, at the moment no, the first use of this was with Roslyn's TextChange which is a struct. It certainly would have made the nullability bits easier though :)

Copy link
Member

Choose a reason for hiding this comment

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

I have no problem with merging this as is. I just wanted to write my thoughts down. We can definitely get back to them later as we go.

@davidwengier davidwengier merged commit 27e921a into dotnet:main Jul 23, 2024
12 checks passed
@davidwengier davidwengier deleted the CohostDocumentHighlight branch July 23, 2024 00:56
@dotnet-policy-service dotnet-policy-service bot added this to the Next milestone Jul 23, 2024
@RikkiGibson RikkiGibson modified the milestones: Next, 17.12 Preview 1 Jul 29, 2024
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.

3 participants