Adding & enhancing client apis over gRPC #244
Merged
Conversation
This file contains 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
This reverts commit ea5dc12.
3 tasks
Merged
3 tasks
rynowak
reviewed
Mar 9, 2020
rynowak
reviewed
Mar 9, 2020
test/Dapr.AspNetCore.IntegrationTest/AppWebApplicationFactory.cs
Outdated
Show resolved
Hide resolved
JamesNK
reviewed
Mar 9, 2020
src/Dapr.Client/DaprClientBuilder.cs
Outdated
| } | ||
| else | ||
| { | ||
| channel = GrpcChannel.ForAddress(this.daprEndpoint, this.gRPCChannelOptions); |
There was a problem hiding this comment.
Could do:
channel = GrpcChannel.ForAddress(this.daprEndpoint, this.gRPCChannelOptions ?? new GrpcChannelOptions());
JamesNK
reviewed
Mar 9, 2020
rynowak
reviewed
Mar 10, 2020
rynowak
reviewed
Mar 10, 2020
| @@ -59,6 +59,13 @@ public abstract class DaprClient | |||
| /// <param name="metadata">A key/value pair to pass to the method to invoke.</param> | |||
| /// <param name="cancellationToken">A <see cref="CancellationToken" /> that can be used to cancel the operation.</param> | |||
| /// <returns>A <see cref="Task" /> that will complete when the operation has completed.</returns> | |||
| /// <remarks>If the target service is listening on http, a key value pair of http.verb and the verb must be added to the metadata parameter. | |||
| /// For example: | |||
rynowak
reviewed
Mar 10, 2020
| @@ -223,7 +226,7 @@ public async ValueTask<StateEntry<TValue>> GetStateEntryAsync<TValue>(string sto | |||
| public abstract ValueTask<bool> TryDeleteStateAsync( | |||
| string storeName, | |||
| string key, | |||
| ETag etag, | |||
| string etag, | |||
There was a problem hiding this comment.
I don't have a big objection to this, but the rationale for making ETag its own type is to make it clear to users that they should not try to guess a good value for an etag. Making it a simple string makes it appear like something users can create.
rynowak
previously approved these changes
Mar 10, 2020
|
@rynowak Could you please approve it again, addressing some of the review comments resetted the review. |
JamesNK
approved these changes
Mar 10, 2020
rynowak
approved these changes
Mar 10, 2020
LMWF
approved these changes
Mar 10, 2020
shalabhms
approved these changes
Mar 10, 2020
Closed
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
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 of Changes:
It will close multiple issues:
#226
#225
#224
#223
#243
#221
#142