-
Notifications
You must be signed in to change notification settings - Fork 479
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
[StyleCop] Fix stylecop warnings on Bot.Builder.TestBot.WebApi #1642
Conversation
Pull Request Test Coverage Report for Build 53587
💛 - Coveralls |
|
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. This is all great stuff.
protected readonly Dialog _dialog; | ||
protected readonly BotState _conversationState; | ||
protected readonly BotState _userState; | ||
protected readonly ILogger _logger; | ||
#pragma warning restore SA1401 // Fields should be private |
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 code is an exact drop of the core bot sample (in this case the webapi sample)
It would be great if we can maintain the consistency, that makes updates much easier. So can we exclude this warning outside of the code in an external rules file. That is possible with stylecop isn't it?
{ | ||
DestinationStepAsync, | ||
OriginStepAsync, | ||
TravelDateStepAsync, | ||
ConfirmStepAsync, | ||
FinalStepAsync, | ||
})); | ||
}; | ||
AddDialog(new WaterfallDialog(nameof(WaterfallDialog), steps)); |
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.
Why?
Can we just keep this consistent with the original sample?
Specifically, sure if you want to make this change for some reason can you also make it in the sample.
{ | ||
InitialStepAsync, | ||
FinalStepAsync, | ||
})); |
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.
ditto
|
||
// The initial child Dialog to run. | ||
InitialDialogId = nameof(WaterfallDialog); | ||
} | ||
|
||
private static Task<bool> DateTimePromptValidator(PromptValidatorContext<IList<DateTimeResolution>> promptContext, CancellationToken cancellationToken) |
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.
Why move this?
The original order was intentional. we can move it if you like but exactly why would we do that?
(and again consistency with the original sample)
Resolve stylecop issues on Bot.Builder.TestBot.WebApi
Changes made: