Axsuarez/skill dialog #757
Conversation
…botbuilder-python into axsuarez/skill-dialog
… into axsuarez/skill-dialog
…e some classes to botbuilder.integration.aiohttp
…sts. Fix SkillDialog.
…alog.begin_dialog
…botbuilder-python into axsuarez/skill-dialog
gabog
left a comment
gabog
left a comment
There was a problem hiding this comment.
Check my comments and see if they are worth addressing.
FYI, I went very light on the samples (I assume those are for dev testing) and also skipped reviewing *aiohttp (not sure I can add any value in reviewing that).
...xperimental/skills-prototypes/dialog-to-dialog/dialog-root-bot/adapter_with_error_handler.py
Outdated
Show resolved
Hide resolved
samples/experimental/skills-prototypes/dialog-to-dialog/dialog-echo-skill-bot/config.py
Outdated
Show resolved
Hide resolved
samples/experimental/skills-prototypes/dialog-to-dialog/dialog-root-bot/config.py
Outdated
Show resolved
Hide resolved
# Conflicts: # libraries/botbuilder-core/botbuilder/core/activity_handler.py # libraries/botbuilder-core/botbuilder/core/adapters/test_adapter.py # libraries/botbuilder-dialogs/botbuilder/dialogs/prompts/oauth_prompt.py # libraries/botbuilder-integration-aiohttp/botbuilder/integration/aiohttp/bot_framework_http_client.py
…he same behavior as C#
…lowing the same behavior as C#
…botbuilder-python into axsuarez/skill-dialog
…lowing the same behavior as C#
carlosscastro
left a comment
carlosscastro
left a comment
There was a problem hiding this comment.
Looks good. Please my mini-rant around constants. Sorry, I know I'm annoying but I'd value consistency on this. Thanks!
| ) | ||
|
|
||
| # Inspect the skill response status | ||
| if not 200 <= response.status <= 299: |
There was a problem hiding this comment.
In all languages we have a statuscodes set of constants, so you can do StatusCodes.OK. Can we have that in python? Not a fan of writing numbers where we could have semantically more meaningful code.
Also, C# has a method on responses called IsSuccessStatusCode which internally checks between 200 and 299. On top of te constants you could do that, optionally. If we do this in at least 2 places, I'd introduce the method right away,
There was a problem hiding this comment.
i had similar feedback on the other review we have constants so we should use them - in fact we seem to be using two different constants for http status code. Having said that - this is obviously a code quality issue rather than functionality - if this is an issue across more of the code don't block on this - just open a R9 bug for global clean up - not a big deal.
|
I didn't check the samples but the rest of the stuff looks good. I'd love @tracyboehrer to take a look since he is more acquainted with python specific things and @gabog since he is the king of skills :). From Gabo's review I think we were already pretty close, so I don't expect much churn, but he may spot subtle details on the latest skill changes. |
johnataylor
left a comment
johnataylor
left a comment
There was a problem hiding this comment.
assuming the etag issue is now gone - i.e. we don't fail on etag ("last one wins" FWIW) I'm approving, however, it would be great if mr skills (aka gabo) signs off.
| ) | ||
|
|
||
| # Inspect the skill response status | ||
| if not 200 <= response.status <= 299: |
There was a problem hiding this comment.
i had similar feedback on the other review we have constants so we should use them - in fact we seem to be using two different constants for http status code. Having said that - this is obviously a code quality issue rather than functionality - if this is an issue across more of the code don't block on this - just open a R9 bug for global clean up - not a big deal.
fixes #665
fixes #831
fixes #825
fixes #834
The features of this PR were e2e tested with the code in #853