#1 Added numberwithunitprompt.py & numberwithtype_prompt.py file#2
#1 Added numberwithunitprompt.py & numberwithtype_prompt.py file#2arafattehsin merged 5 commits intoBotBuilderCommunity:masterfrom rvinothrajendran:master
Conversation
|
Looks good to me. Thanks @rvinothrajendran for the PR. |
numberwithtype_prompt class added
rvinothrajendran
changed the title
|
@arafattehsin thanks for you to approved the PR . How to merge code change since I don't have write access |
|
I'll be merging it soon. Just finalizing a few things. |
|
I would like to see some unit tests added before merging, if at all possible, even if you just duplicated the tests from the JS library. A good package is pytest. We could then add the coverage package for code coverage when we get a pipeline set up. |
|
Totally agree with that! |
garypretty
left a comment
garypretty
left a comment
There was a problem hiding this comment.
Thank you very much for your contribution - our first fro the Python repo!
As this is the first we need to ensure we start our naming correctly. Would you mind altering the folder structure to reflect that these are Bot Builder Community packages.
e.g. change 'botbuilder-dialogs-prompts' to 'botbuilder-community-dialogs-prompts'
Thanks again!
garypretty
left a comment
garypretty
left a comment
There was a problem hiding this comment.
It would also be great if you could add a readme for the prompts package, similar to this one in the JS repo.... https://github.com/BotBuilderCommunity/botbuilder-community-js/tree/master/libraries/botbuilder-dialog-prompts
|
@garypretty I think he just followed the NodeJS packages, and all ours are "botbuilder-". I've been contemplating removing the "botbuilder-" completely from the NodeJS folder structure since it's actually in a Bot Builder Community package... Would cut down on verbosity. I'm open to either. @rvinothrajendran I would definitely suggest following Python best practices for file and variable names. I think Python developers primarily like snake case. |
|
Thanks @garypretty @szul sure will follow Python best practices |
|
Thanks @rvinothrajendran @szul I hadn't spotted it was the same as JS, so thanks for pointing that out. I am happy to remove the botbuilder if it makes things less verbose for folder names. The one thing I want to ensure, hence this original comment, is that our packages are consistently showing botbuilder community so that we do not risk being confused with any official packages and we also maintain the connection to the project. |
|
@garypretty The NPM packages are scoped to “botbuildercommunity”. |
Folder name change to botbuilder-community-dialogs-prompts unit test script has added
|
@arafattehsin @szul @garypretty , Kindly review the changes |
szul
left a comment
szul
left a comment
There was a problem hiding this comment.
This is good. We will need to get the CI/CD squared away at some point, but I will add a bunch of issues separate of this PR to address those.
|
@arafattehsin I'm good with this. You can be gatekeeper for the merge. I'll do some research into Python package structures and CI/CD and then load up the issues load so we can get the maturity on this repo up to the level of the others. |
|
Sure. I'll take care of this. |
default locale condition handle in the ctor & removed from the on_recognize function
|
For now, I have merged it with master as we did not want to hold this PR. However, from now on, just like our other repos, develop will be used as the default branch. |
4 participants
#1 Converted C# Prompt:NumberWithUnitPrompt class to Python class
#3 Converted Port numberWithType JS to Python
two files has added in the same PR #2