Update conversational-context.md #57
Conversation
All of the recommended changes below are for the "Using context to enable Intents" part of the guide. Recommendation 1: The speech for def handle_yes_honey_intent Since self.milk = False, milk must return False in the def handle_yes_honey_intent when one refuses Milk. I also tried it out in practice and the speech needed to be flipped so that everything lines up with the correct answer at the end. Recommendation 2: The single apostrophe in the speech The single apostrophes in both the def handle_no_honey_intent and def handle_yes_honey_intent for the phrase "Here's" was causing problems in python. Nothing would work until I changed the "Here's" to "Heres" maintaining single apostrophe throughout. I thought it simpler just to remove the single apostrophe, but maybe the convention should be double apostrophes for the self.speak. Recommendation 3: The import library I thought it would be useful to have the library imported for contexts in the guide. Simply invoking adds_context or removes_context without the import library did not work. I was able to find it however in the Mycroft-Core (I think), but that was difficult as a newbie.
There was a problem hiding this comment.
Thanks so much @Quinn2017 for submitting this PR for our docs-rewrite repo, we really appreciate it. I've reviewed this PR and your recommendations. I'd like to merge this PR in then tackle the recommendations separately.
Before I can merge the PR, I need to request a Contributor Licensing Agreement so that we can maintain our open source licensing compliance.
Would you mind providing one of these, then I'll be able to merge in the Pull Request?
Again, really appreciate the PR.
Kind regards, Kathy
|
Waiting on CLA before merging. |
|
@KathyReid I filled out the CLA right away (a week ago), but nothing has come in my email box since (that's including the spam email folder). I figured the team may have been busy or changes were being undertaking on the CLA forms. |
|
@Quinn2017 thanks for getting on the CLA so quickly! I'll follow up internally and come back to you 👍 |
|
Hi @Quinn2017, unfortunately we had a minor issue with our CLA form last week, and your submission hasn't come through - my apologies. Could I please get you to fill out the form again? Sorry for the inconvenience. |
penrods
added
the
CLA: Yes
|
CLA received with thanks, merging PR. |
All the recommended changes are for the "Using context to enable Intents" part of the "conversational-context.md" guide.
Recommendation 1: The speech for def handle_yes_honey_intent
Since self.milk = False, milk must return False in the def handle_yes_honey_intent when one refuses Milk. I also tried it out in practice and the speech needed to be flipped in this handle so that everything lines up with the correct answer at the end. I think what happened is that a mix-up occurred when writing the instructions originally as the handle_no_milk_intent is placed under the YesMilkIntent and vice versa, which might have caused things to get turned around. For the sake of simplicity, I did not modify this in my pull request.
Recommendation 2: The single apostrophe in the speech
The single apostrophes in both the def handle_no_honey_intent and def handle_yes_honey_intent for the part of the phrase "Here's" was causing problems in python. Nothing would work until I changed the "Here's" to "Heres" while still maintaining single apostrophe throughout. I thought it simpler just to remove the single apostrophe here, but perhaps the convention should be double apostrophes for the self.speak so that "here's" doesn't cause problems.
Recommendation 3: The import library
I thought it would be useful to have the necessary library import for contexts included in the guide. Simply invoking adds_context or removes_context without the import library did not work for me. I was able to find it however in the Mycroft-Core (if I recall correctly), but that was difficult as a newbie.
Note:
I also had an issue with the order of the adds_context and removes_context, but as I already mentioned it to Kathy (in the Chat Room), I'll just open an issue in Github for that one. I had to reverse the order, placing adds_context first and removes_context second for it to work.
Mycroft Version
Using Picroft, Mycroft Core ver. 18.2.4 beta