search

MycroftAI / adapt

Adapt Intent Parser
Apache License 2.0
712 stars 154 forks source link

Suggestion: Register entity takes in lists or a single string of entity_value #82

Closed OAkyildiz closed 2 years ago

OAkyildiz commented 6 years ago

In the IntentDeterminationEngine.register_entity(...) registering intent in for loops in the user's code does not look pythonic and interacting that way with the API does not look refined to the end user.

In the following pull request, the iteration of entity_values is internalized the iteration in a simple manner. It is a minor change, the iteration can also be parallelized, but it can be applied to similar methods to avoid external for loops from becoming the formal way of interacting with the API.

clusterfudge commented 6 years ago

It appears I made a painful choice back in the day on the method signature (entity_value, entity_type). Because these are positional and untyped, reversing them would be a breaking change to the API, to be discovered at runtime and super opaque (even the boldest disclaimers in

We could add an additional method (register_entities) to try to fix this semantic, but the ordering would be inconsistent in an unpleasant way.

I'm happy to review a PR in this vein!

OAkyildiz commented 6 years ago

I simply put the for loop into the function. Not paralellized, (well, probably no need for that), no type check for string but the tests are passing. Also updated the respective @property and added an example.

clusterfudge commented 2 years ago

This hasn't seen any forward progress, and I'm not super to pursue it myself (as a small ergonomic change). @OAkyildiz if you have the time to wrap up your PR, please indicate as much, otherwise I'll be closing this on 9/17 as out of scope/stale for 1.0.0.

Thanks!