treethought / flask-assistant

Framework for Building Virtual Assistants with Dialogflow and python
Apache License 2.0
379 stars 101 forks source link

Remove unused api attribute of Assistant class #75

Closed gmolau closed 6 years ago

gmolau commented 6 years ago

The main concern of this PR is this line from the Assistant classes init method:

self.api = ApiAi(dev_token,client_token)

This api attribute is used nowhere in the entire framework, but does produce a warning if the Dialogflow developer access token is not set (this one). This warning is very annoying and since we shouldn't have an unused class attribute anyway I would suggest to remove the attribute.

Also fixes some typos in the class documentation and adds the venv/ dir to .gitignore.

All tests pass as before.

treethought commented 6 years ago

Hey @gmolau, I apologize for my late response.

While the Assistant's api attribute is not used in the core functionality, it is accessed when registering an Assistant with Dialogflow. Each Assistant is given an individual Dialogflow API object to allow blueprints and multiple assistants https://github.com/treethought/flask-assistant/pull/50.

I do agree the warning is annoying. I think we can raise the warning in a more appropriate manner.

gmolau commented 6 years ago

Are you sure about that? I can see that the SchemaHandler class is initialized with an Assistant object (here), but none of the methods called on that object seem to have any reference to the Assistant.api attribute.

The SchemaHandler does have its own SchemaHandler.api attribute (from here), and that one is used in some of the classes methods. Could it be that you have confused the ApiAi instance of the Assistant class with that of the SchemaHandler class? Or am I misunderstanding how this all works?

treethought commented 6 years ago

No you're correct. Good catch. After making my comment I realized that using the Assistant.api inside SchemaHandler was never actually merged into master. I've just pushed the change

gmolau commented 6 years ago

Ok, but that still leaves the problem that every instance of Assistant has an ApiAi attribute that most of them don't need. Considering that the Assistant handles every single request whereas the ApiAi is only used during development I would find that rather inelegant. Wouldn't it be possible to only pass the ApiAi instance in the constructor when it is actually needed (i.e. in SchemaHandler)?

treethought commented 6 years ago

you have a good point, and I'm definitely open to handle the ApiAi instance differently.

However, like you said the ApiAi instance is only used during development and primarily through the CLI. Most users don't interact with the ApiAi or SchemaHandler classes directly, and only use the Assistant class.

Previously the access tokens for Dialogflow were only declared as environment variables, which wouldn't require linking the ApiAi to the Assistant class. But linking an api instance to an Assistant becomes necessary when a project has multiple assistants/blueprints.

What do you think about using some type of config file to associate access tokens to an Assistant? That way SchemaHandler can simply create a new ApiAi instance after looking up the given Assistant's tokens?

gmolau commented 6 years ago

Yes, having the CLI tools mapped to specific assistants via a config file sounds like a good idea. I have never used the CLI, so I can't say what would be a good way to implement this, but having a clear separation between the backend management tools and the actual assistant seems very desirable.