search

Watson-Personal-Assistant / SkillBoilerplate

Other
7 stars 63 forks source link

removed support for 'skill' in manifest.nlu #27

Closed offerakrabi closed 6 years ago

offerakrabi commented 6 years ago

removed checks for 'skill' as one of the nlu engines (added an error in case it's still there). Tested manually seems to work fine.

git issue: https://github.ibm.com/ConsumerIoT/WA-dev-issues/issues/324

erezbi commented 6 years ago

@offerakrabi you almost got me there. this should not be merged yet! We first need to merge pre-release to master as part of the May release.

offerakrabi commented 6 years ago

updated error message

erezbi commented 6 years ago

@offerakrabi Code looks fine. What testing did you run on this?

offerakrabi commented 6 years ago

@erezbi tested this manually. it's a problem testing the boilerplate because it requires nisha to update all the skill in the testing framework

erezbi commented 6 years ago

We need to test all of the relevant cases such as nlu value being: only 'skill'. empty. Skill and some other nlu. no skill and only other nlu. We should automate these tests.

troyibm commented 6 years ago

I think the manifest.json nlu property should have "skill" in it. This is so skills will continue to work if the env variable is set for the Core to do NLU. Once there is no possibility that the Core handles NLU, then "skill" should be removed from the nlu property in the boilerplate. @erezbi @offerakrabi

offerakrabi commented 6 years ago

@troyibm Added 'skill' to manifest. @erezbi I checked all these scenarios