Skip to content

Conversation

@tmat
Copy link
Member

@tmat tmat commented Dec 5, 2019

Unifies the pattern used to run a feature in OOP and applies nullable annotations.

This reflects the fact that the remote call may fail and return null, which previously wasn't consistently handled across all features. If it happens we should fall back to running in-proc.

FeatureEntryPoint()
{
  var client = await RemoteHostClient.TryGetClient();
  if (client != null) { var result = client.RemoteCall(); if (result.HasValue) return result.Value; } 
  FeatureImplInProc(); 
}

Also reduces the amount of overloads and turns extension methods used for remote invocation into instance methods defined directly on RemoteHostClient type.

Copy link
Member

@CyrusNajmabadi CyrusNajmabadi left a comment

Choose a reason for hiding this comment

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

comment

Copy link
Member

@CyrusNajmabadi CyrusNajmabadi left a comment

Choose a reason for hiding this comment

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

comment.

@tmat tmat changed the title Remote calls Unify the pattern used to run a feature in OOP Dec 6, 2019
@tmat tmat marked this pull request as ready for review December 7, 2019 02:49
@tmat tmat requested a review from a team as a code owner December 7, 2019 02:49
@tmat tmat requested a review from a team December 7, 2019 02:49
@tmat
Copy link
Member Author

tmat commented Dec 7, 2019

@dotnet/roslyn-ide PTAL

Copy link
Contributor

@ryzngard ryzngard left a comment

Choose a reason for hiding this comment

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

Looks reasonable to me

Copy link
Member

@CyrusNajmabadi CyrusNajmabadi left a comment

Choose a reason for hiding this comment

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

c

@CyrusNajmabadi
Copy link
Member

Regardless, we should likely take the conversation about enforced styles to some other more suitable location.

@tmat tmat force-pushed the RemoteCalls branch 3 times, most recently from d656d0d to 492a88d Compare December 17, 2019 00:17
Copy link
Contributor

@sharwell sharwell left a comment

Choose a reason for hiding this comment

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

Done with 29/43 files

@CyrusNajmabadi
Copy link
Member

Small concerns. Nothing major though.

@JoeRobich
Copy link
Member

All checks have passed. Overriding to merge.

@JoeRobich JoeRobich merged commit 1331a9d into dotnet:master Dec 18, 2019
@jinujoseph jinujoseph added this to the 16.5.P2 milestone Dec 18, 2019
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.

6 participants