search

OneBusAway / onebusaway-alexa

An Java-based app to communicate with Amazon Alexa for devices such as the Amazon Echo
Other
52 stars 18 forks source link

Fix #14 - Support all regions via Regions API #21

Closed barbeau closed 8 years ago

barbeau commented 8 years ago

Please don't merge

Initial work in progress for improving region discovery. Note that I modified the onebusaway-client-library to include some new functionality for finding only "nearby" regions (https://github.com/OneBusAway/onebusaway-client-library/issues/7), so you'll need to update your local Maven repo for this to compile.

Note that I haven't actually tested this yet, but most of the infrastructure should now be in place.

See below - PR has been updated and ready to merge.

barbeau commented 8 years ago

Alright, I've tested this and it works with Tampa stops now as well. I have a few more items of cleanup to do and then I'll ping again when it's ready for review.

barbeau commented 8 years ago

@philipmw Ok, this PR is review for review/merge. I've squashed the commits and force-pushed the merge-ready commit into this same PR.

philipmw commented 8 years ago

Overall I am delighted to have this PR! I am glad we are able to remove the hacky ObaAgencies.

barbeau commented 8 years ago

@philipmw Ok, I believe I addressed all the feedback. I'm new to Java's Optional<> - fun :).

As far as handling a null value for the obaBaseUrl - I've updated the flow to ask for the city again in this case, so we should never persist a region without this information. tl;dr - for now we can assume this shouldn't happen (this is a required field in the Regions API for the apps, currently). Please take a look and make sure this corresponds with your thoughts on this. I'll also think about how we might need to change the oba-client-library to support future cases that could arise.

barbeau commented 8 years ago

Alright, updated. Throwing the uri exception cascaded several levels and I wasn't sure the best way to finally handle it in the Speechlet - check it out and let me know what you think. I updated this to also use the test framework from your last commit, added a Tampa test as well.