Skip to content
This repository was archived by the owner on May 6, 2022. It is now read-only.

Conversation

@timmywil
Copy link
Member

@timmywil timmywil commented Jan 7, 2021

BREAKING CHANGE: The NLU slot type was changed in version 5.0. This commit changes
it back to what it was pre-5.0. slots will be a map, and the property names will
be derived from the name given in the NLU metadata.

This also adds Minecraft NLU to the example app.

BREAKING CHANGE: The NLU slot type was changed in version 5.0. This commit changes
it back to what it was pre-5.0. slots will be a map, and the property names will
be derived from the name given in the NLU metadata.
@timmywil timmywil requested a review from space-pope January 7, 2021 19:23
Copy link
Contributor

@space-pope space-pope left a comment

Choose a reason for hiding this comment

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

Nice work; just have one question about the typing of value. The RN type declares it to be a string, but the Android code retains the Java type (as much as possible, at least). The latter approach would make RN behave most like the native APIs.

@timmywil
Copy link
Member Author

timmywil commented Jan 8, 2021

In case there was any confusion, edits to the API docs in the README should mainly be done in types.ts rather than in the README itself as the README is generated from the types.

@space-pope
Copy link
Contributor

I thought it was autogenerated but forgot to track down the origin while I was reading (despite it being listed right in the file...)

Copy link
Contributor

@space-pope space-pope left a comment

Choose a reason for hiding this comment

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

LGTM

@timmywil timmywil merged commit 001402a into master Jan 11, 2021
@timmywil timmywil deleted the feature/nlu branch January 11, 2021 20:45
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

4 participants