Skip to content
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

Update virtual assistants v2 #5137

Merged
merged 66 commits into from
Dec 6, 2019

Conversation

inventor96
Copy link
Contributor

Second try after tests failed with #4980. This time the branch is rebased off of the dev branch.

@inventor96
Copy link
Contributor Author

@tanja3981 I've added some missing usage of translate(). Not sure if that's what you were going to change, or if you were just fixing incorrect translations.

@nightscout nightscout deleted a comment Nov 5, 2019
@nightscout nightscout deleted a comment Nov 5, 2019
@nightscout nightscout deleted a comment Nov 5, 2019
@nightscout nightscout deleted a comment Nov 5, 2019
@inventor96
Copy link
Contributor Author

@PieterGit @sulkaharo Other than @tanja3981 we don't seem to be getting a lot of feedback on this. :-( Would it be appropriate to request for a few individuals in the "CGM In the Cloud" FB group to help test it, and write up a quick instruction set for those who don't normally do dev testing?

@sulkaharo
Copy link
Member

@inventor96 sorry for the lag! Any additional testing would be great - I personally don't have a Home or Alexa device and am a bit useless in real world testing of this

@nightscout nightscout deleted a comment Nov 10, 2019
@inventor96
Copy link
Contributor Author

@sulkaharo I don't know if you've been watching the post in the FB group, but a handful of people have volunteered to help test. Is there a deadline I should give them (conditional upon me resolving any issues they find) so we can keep this moving forward?

@sulkaharo
Copy link
Member

@inventor96 This is one of the last things we could merge in to get the next release done, so the sooner the better. I don't think there's need to have a hard deadline, but if you can get some people to test this asap so we get a feel of how much polish / fixes there are and thus a potential timeline for when this is finished, that'd be great. <3

@@ -242,108 +89,67 @@ After you enable testing, you can also use the Alexa Simulator in the left colum

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Indicate on the above lines somewhere (line 235-241). You can not type the keyword "Alexa" in the devloper console Test tab. This will confuse the alexa simulator. Instead just type out "Ask night scout how I am doing"

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Not a bad idea...

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

It took me two days to figure this out. I believe if you use a mic and ask "Alexa, ask night scout how I am doing" It also gets confused. I was just copying and pasting from the suggested commands, which does not work.

@bennettnicholas
Copy link

bennettnicholas commented Nov 19, 2019

@inventor96 I am fairly new to github, and since there is no way to PM on github. I have to ask a "dumb" question. How can I merge your branch into my current branch? I know how to do it usually, but your branch is not displayed on the master.

@inventor96
Copy link
Contributor Author

@bennettnicholas Check out these instructions I've sent to members of the "CGM in the Cloud" Facebook group: https://docs.google.com/document/d/1v8EAbg7P7P_t-k2lOkZyUz7A7TpZmagq-ofma48FG4E/edit?usp=sharing

It's probably not the best way to do it, but it allows people to test the branch without the need to run git commands on the repo locally on a computer. If you're comfortable using git on your computer, I believe you would add a remote that points to my repo, create a new branch in your local repo, then fetch and merge this PR into that new branch. (Disclaimer: it's been a while since I've done this, so you might do some Googling to confirm those steps)

@inventor96
Copy link
Contributor Author

@sulkaharo Over the past two weeks, I've had 3 Alexa users and 1 Google Assistant user try out this PR (with a couple more that haven't responded yet), and the only issues reported have not been related to the code; just things like their chosen skill/action name is hard for their virtual assistant to recognize. Do you have an NS version name I can add to the Google Assistant documentation and then the PR can be merged?

@jasoncalabrese
Copy link
Member

Codacy Here is an overview of what got changed by this pull request:

Issues
======
- Added 2
           

Complexity increasing per file
==============================
- lib/api/index.js  1
- lib/api/googlehome/index.js  4
- lib/api/alexa/index.js  8
- lib/plugins/googlehome.js  6
         

Complexity decreasing per file
==============================
+ lib/plugins/alexa.js  -1
         

See the complete overview on Codacy

@Adamwesleyo
Copy link

I'm currently testing the Google home version and it connects well and communicates well thus far. I'm going to branch it to my phone so I can use it remotely as well as at home.

@alphacentauri82
Copy link

Testing with google home works very well! I love it!

@inventor96
Copy link
Contributor Author

@sulkaharo @PieterGit We're now up to 3 on each platform that have confirmed successful results. Do we have version name for 0.13? And then this PR can be merged?

@sulkaharo sulkaharo merged commit c153a96 into nightscout:dev Dec 6, 2019
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

7 participants