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

✨ Access nlu properties in CoreRequest #1271

Merged
merged 10 commits into from
Apr 25, 2022

Conversation

rubenaeg
Copy link
Contributor

Proposed Changes

This PR modifies the JovoInputBuilder to accept an input object, analogous to JovoInput. This allows inheriting request classes to easily extend the getInput() function, as seen in CoreRequest. Furthermore, CoreRequest.prototype.getIntents() and CoreRequest.prototype.getEntities() allow access to properties in input.nlu.

Types of Changes

  • Bug fix (non-breaking change which fixes an issue)
  • New feature (non-breaking change which adds functionality)
  • Breaking change (fix or feature that would cause existing functionality to change)

Checklist

  • My code follows the code style of this project
  • My change requires a change to the documentation
  • I have updated the documentation accordingly
  • I have added tests to cover my changes
  • All new and existing tests passed

@rubenaeg
Copy link
Contributor Author

Oh, I was just wondering, would it make sense to change getEntities() (and getIntents() for that matter as well) to merge the properties instead of using logical or ||?

@rubenaeg rubenaeg changed the title Fix/input nlu access ✨ Allow access of nlu properties Mar 30, 2022
@@ -18,8 +25,12 @@ export class CoreRequest extends JovoRequest {
return this.locale;
}

getInput(): JovoInput {
return new JovoInputBuilder(super.getInput()).set('nlu', this.input?.nlu).build();
Copy link
Member

Choose a reason for hiding this comment

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

Could you explain a bit further what this is doing here? Thanks!

Copy link
Contributor Author

Choose a reason for hiding this comment

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

So the idea behind this was that you can easily extend this.$input by allowing inheriting request classes to build upon the JovoInputBuilder. Here we get the input built in JovoRequest and add the nlu property, so you can access this.$input.nlu in the handler.

Copy link
Member

Choose a reason for hiding this comment

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

Thank you. I'm still not sure if I get it, to be honest. A bit confused why specifically nlu needs to be set in this method and why we're not setting any other property. What is a use case here?

Could the reasoning be commented or documented somewhere? i think otherwise it could happen that we don't know why this is there if we look at the code in a few months

Copy link
Contributor Author

@rubenaeg rubenaeg Apr 19, 2022

Choose a reason for hiding this comment

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

Okay so I guess this could be seen as being redundant, the original idea was to access the nlu properties in the handler, because if intents/entities were set in input.nlu, you had no way of accessing it with this.$input, but since this PR allows accessing them through getIntent() and getEntities() it can be removed.

Copy link
Contributor

Choose a reason for hiding this comment

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

@rubenaeg How do we access other properties that we might add to $input.nlu?
This could be an output object with messages if the SLU/NLU provides dialog management or messages.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Well I now removed the code, but you can see in the code above how you could add properties to the input object that is returned by getInput(). @jankoenig do you need more input on this PR? I can write some documentation if you'd like, something like "Extending the Input object", where I could also cover ambient declarations...

@rubenaeg rubenaeg changed the title ✨ Allow access of nlu properties ✨ Access nlu properties in CoreRequest Apr 25, 2022
@jankoenig jankoenig self-requested a review April 25, 2022 13:38
@jankoenig jankoenig merged commit d057287 into jovotech:v4/dev Apr 25, 2022
@ngocht ngocht deleted the fix/input-nlu-access branch January 11, 2023 09:11
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

6 participants