-
Notifications
You must be signed in to change notification settings - Fork 530
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
Csharp conversation API quickstarts #1144
Csharp conversation API quickstarts #1144
Conversation
Signed-off-by: Alice Gibbons <alice@diagrid.io>
{ | ||
const string prompt = "What is dapr?"; | ||
|
||
var builder = WebApplication.CreateBuilder(args); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@WhitWaldo can you review the Conversation Client instantiation here? Should I do it a different way?
Also, is it weird putting the builder/app in a Class? Can remove that...
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This is precisely right. As this is configured to use a "Microsoft.NET.Sdk.Web" project, this is the appropriate way to set up the "app builder" itself. Dapr then gets registered on line 28, DI finalized on line 29 and the Dapr client instantiated from that DI pipeline on 32.
Technically this could be shortened a little bit as single-file C# apps don't as much of the window dressing any longer, but this is fine - if you want to shorten it, remove lines 19, 20 and 54 (class Program { }
)
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
The rest of it looks great
var builder = WebApplication.CreateBuilder(args); | ||
builder.Services.AddDaprConversationClient(); | ||
var app = builder.Build(); | ||
|
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Missing a line here: await app.RunAsync();
- while not strictly necessary since we don't need the whole web server in this example, it'd be a good practice to add it.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Thanks for checking this out! This got merged but will add this feedback into my next PR
{ | ||
const string prompt = "What is dapr?"; | ||
|
||
var builder = WebApplication.CreateBuilder(args); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This is precisely right. As this is configured to use a "Microsoft.NET.Sdk.Web" project, this is the appropriate way to set up the "app builder" itself. Dapr then gets registered on line 28, DI finalized on line 29 and the Dapr client instantiated from that DI pipeline on 32.
Technically this could be shortened a little bit as single-file C# apps don't as much of the window dressing any longer, but this is fine - if you want to shorten it, remove lines 19, 20 and 54 (class Program { }
)
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
LGTM. I like the consistency across the languages and the feature to more dynamically take inputs
Thank you to both @WhitWaldo and @alicejgibbons for this great contribution and collab. |
Description
Issue reference
We strive to have all PRs being opened based on an issue, where the problem or feature have been discussed prior to implementation.
Please reference the issue this PR will close: #1103 #1105
Checklist
Please make sure you've completed the relevant tasks for this PR, out of the following list: