-
Notifications
You must be signed in to change notification settings - Fork 4
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
Add some options to control "chattiness" of the plugin #40
Conversation
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Hi, thank you for this contribution! I appreciate it very much.
I'm not opposed to anything here, but would you please add a log message if verbose
is True, as there will be no indication to the user if their request failed or succeeded without that. Since this is an async process that hits the PHAL plugin, which may or may not ever respond, that log would make troubleshooting issues much easier. self.log.info()
level should be fine.
6be2c7f
to
8f0a4ee
Compare
Makes sense, thank you for feedback! Now it will be phrase or log entry. |
Generally, I prefer new commits, as it makes it more clear what the history of the PR was. We squash commits on merge so there's no need to force-push anything |
I added two options to your plugin:
acknowledge
dialogue completely (I believe it is have not too much of useful information, because just after it another dialog started which actually duplicates information from first one. In fact, I'd suggest to get rid of it completely). Option disabled by default.Settings file example with new params:
Fixes #34