Bots: Add metadata scheme for bots. #66

neiljp · 2017-08-15T17:57:51Z

Name & (short) description
Whether to use any default commands (default to enabled):
(about, '' (empty), usage [remove?], help/commands)
Provide a list of user commands, which help/commands uses

This is an alternative/supplement/extension of #58, which would also resolv #55.

I also raised the idea of bot metadata in #integrations.

Currently this just provides default-commands which use the simple metadata and (optionally) a list of bot commands provided by the developer. It could allow the user to provide callbacks directly, with the handle_message in the bot only being used for more complex input (like zulip/zulip#5047)

roberthoenig

This will be great! Left a couple stylistic comments. Oh, and there are some linter errors ;)

roberthoenig · 2017-08-16T08:41:34Z

zulip_bots/zulip_bots/bots/wikipedia/wikipedia.py

@@ -18,6 +18,11 @@ class WikipediaHandler(object):
    kind of external issue tracker as well.
    '''

+    META = {'name': 'Wikipedia',
+            'description': 'Searches Wikipedia for a term and returns the top article.',
+            'no_defaults': True,  # Let bot handle all messages


'defaults' might suit here as a name, to avoid negation in parameter names.

Amended, pending push.

roberthoenig · 2017-08-16T08:42:32Z

zulip_bots/zulip_bots/bots/xkcd/xkcd.py

@@ -6,6 +6,8 @@
 XKCD_TEMPLATE_URL = 'https://xkcd.com/%s/info.0.json'
 LATEST_XKCD_URL = 'https://xkcd.com/info.0.json'

+from collections import OrderedDict


Nit: should this go to the other import statements?

Amended, pending push.

roberthoenig · 2017-08-16T08:52:52Z

zulip_bots/zulip_bots/lib.py

+        ('usage', ((lambda: message_handler.usage(), "Bot-provided usage text"))),
+        ('help', (lambda: "{}\n{}\n{}".format(def_about(), message_handler.usage(), def_help()),
+                 "This help text")),
+        ('commands', (def_commands, "A short list of supported commands"))


Hrmm, this construction is fun, but looks a little brittle.

The outer construction is the form of the OrderedDict constructor, the value is simply lambda+Text (so tuple seems ok?); are you referring to the formatting, the data-structure, or some other aspect?

roberthoenig · 2017-08-16T08:53:29Z

zulip_bots/zulip_bots/lib.py

+                          for k, v in bot_details['commands'].items() if k != ''))
+    def def_commands():
+        return "**Commands**: {} {}".format(
+            " ".join(k for k in default_commands if k != ''),


if k suffices.

Same for the other occurrences.

roberthoenig · 2017-08-16T08:57:23Z

zulip_bots/zulip_bots/lib.py

+    def def_about():
+        desc = bot_details['description']
+        return "**{}**{}".format(bot_details['name'],
+                                 "" if desc == "" else ": {}".format(desc))


return "**{}**{}".format(bot_details['name'], not desc or ": {}".format(desc)) ?

Interesting construction; I just played with this but it doesn't seem to work in practice as "not desc" gives True :)

Alternatively, just:

if bot_details['description'] == "": return "**{}**".format(bot_details['name']) return "**{}**: {}".format(bot_details['name'], bot_details['description'])

?

roberthoenig · 2017-08-16T09:01:21Z

zulip_bots/zulip_bots/lib.py

+    ])
+    # Update bot_details from those in class, if present
+    try:
+        bot_details.update(lib_module.handler_class.META)


bot_details.update(getattr(lib_module.handler_class, 'META'. {}) ?

roberthoenig · 2017-08-16T09:10:21Z

zulip_bots/zulip_bots/lib.py

+                for command in default_commands:
+                    if command == '':
+                        continue
+                    if message['content'].startswith(command):


Maybe we want a simple equality check here, so we can simplify the whole # Handle any default_commands first if clause?

To clarify, do you mean something like if default_commands == {}:, or even if default_commands: ?

Yeah, I think we want an equality check, not startswith. It also means '' doesn't need to be a special case.

roberthoenig · 2017-08-16T09:11:27Z

zulip_bots/zulip_bots/lib.py

+        return "**Commands**: {} {}".format(
+            " ".join(k for k in default_commands if k != ''),
+            " ".join(k for k in bot_details['commands'] if k != ''))
+    default_commands = OrderedDict([


Why does this need to be an ordered dict?

If these are not ordered, then when iterated in the commands and help text, the order is not guaranteed - perhaps not vital for regular use, but tests won't be consistent either!

roberthoenig · 2017-08-16T09:12:51Z

zulip_bots/zulip_bots/lib.py

+                    del default_commands[command]
+    # Sync default_commands changes with bot_details
+    if len(default_commands) == 0:
+        bot_details['no_defaults'] = True


I've not entirely looked through this bot_detail default_commands code logic. Maybe this would be easier to understand if we outsource it to a separate function?

neiljp · 2017-08-16T17:03:33Z

Updated to reflect most review comments; pending are final if statement and linter (since code has changed).

timabbott · 2017-08-17T17:17:32Z

zulip_bots/zulip_bots/bots/xkcd/xkcd.py

+                ('latest', "Show the latest comic strip"),
+                ('random', "Show a random comic strip"),
+                ('<comic id>', "Show a comic strip with a specific 'comic id'"),
+        ])  # NOTE: help not listed here, so default command used


I don't understand this comment.

In this bot, with this PR, help is now handled internally by the lib, so the default command is used instead as it stands. If 'help' was listed in commands then the bot wouldn't handle it and the help text would be as specified in the tuple.

timabbott · 2017-08-17T17:22:08Z

zulip_bots/zulip_bots/lib.py

+    return defaults
+
+def sync_botdetails_defaultcommands(bot_details, default_commands):
+    # Update default_commands from any changes in bot_details


I don't understand this function; is it just "updating" in the sense that it's overriding the global defaults with this particular bot's settings?

I broke out the code which adjusted default_commands based on bot_details (in some cases), but also have the last conditional which set default_commands_enabled. That's only (currently) used in that function, so could be cut, but that's why I prefixed it with 'sync'.

timabbott · 2017-08-17T17:22:57Z

zulip_bots/zulip_bots/bots/wikipedia/wikipedia.py

+    META = {
+        'name': 'Wikipedia',
+        'description': 'Searches Wikipedia for a term and returns the top article.',
+        'defaults': False,  # Let bot handle all messages


I think the fact that you feel the need for a comment here suggests we might want a more verbose name than "defaults". I think "default_commands_enabled" would be a good name.

timabbott · 2017-08-17T17:28:27Z

zulip_bots/zulip_bots/lib.py

+    def def_about():
+        if bot_details['description'] == "":
+            return "**{}**".format(bot_details['name'])
+        return "**{}**: {}".format(bot_details['name'], bot_details['description'])


This doesn't feel particularly readable. A better approach might be to write these as as e.g.

"**{name}**: {description}".format(**bot_details)

and probably even better would be e.g.

`"%(name)s: %(description)s" % bot_details

timabbott · 2017-08-17T17:30:48Z

zulip_bots/zulip_bots/lib.py

+        return ("\n".join("**{}** - {}".format(k, v[1])
+                          for k, v in defaults.items() if k) + "\n" +
+                "\n".join("**{}** - {}".format(k, v)
+                          for k, v in bot_details['commands'].items() if k))


Let's use more verbose variable names than k/v here. I think we want "command_name" and "command_description".

I think some of the other new functions in this file could use the same sort of work.

As a general rule, one should basically always be verbose in variable names. Brevity doesn't provide much value, and the extra clarity is huge.

Also, put the newline on its own line so it is more visible.

I've generally been wary of verbosity here (not my code), perhaps because I'm aware how verbose I've been in my own code! (and I've generally gone very verbose, and wanted to fit in shortish lines here).

I've gone for variables without the command_ prefix, which I think provides a balanced approach.

The newline was on it's own line in a previous version; moved it back :)

timabbott · 2017-08-17T17:31:42Z

zulip_bots/zulip_bots/lib.py

+        ('about', (def_about, "The type and use of this bot")),
+        ('usage', ((lambda: message_handler.usage(), "Bot-provided usage text"))),
+        ('help', (lambda: "{}\n{}\n{}".format(def_about(), message_handler.usage(), def_help()),
+                  "This help text")),


I think the lambdas here aren't super readable; I'd rather those have an actual def method like the others.

timabbott · 2017-08-17T17:33:21Z

zulip_bots/zulip_bots/lib.py

+                    del default_commands[command]
+    # Sync default_commands changes with bot_details
+    if len(default_commands) == 0:
+        bot_details['defaults'] = False


This function would be much better as something that returned a new dictionary based on the inputs, rather than mutating the default_commands dict. In addition to being a cleaner flow, I'm pretty sure the mutation means this code ends up being broken if multiple bots with different command sets run in the same process.

neiljp · 2017-08-23T01:31:40Z

@timabbott Changes made in a new commit to show the changes separately, though will squash or split as necessary.

zulipbot · 2017-08-23T05:27:20Z

Hello @neiljp, it seems like you have referenced an issue in your pull request, but you have not referenced that issue in your commit message(s). When you reference an issue in a commit message, it automatically closes the corresponding issue when the commit is merged.

Please run git commit --amend in your command line client to amend your commit message description with "Fixes #[issue number]”.

An example of a correctly-formatted commit:

commit fabd5e450374c8dde65ec35f02140383940fe146
Author: zulipbot
Date:   Sat Mar 18 13:42:40 2017 -0700

    pull requests: Check PR commits reference when issue is referenced.

    Fixes #51

Thank you for your contributions to Zulip!

neiljp · 2017-08-27T05:33:01Z

Current status is: (copying from zulip, to keep the context)

I've mostly updated the metadata PR, and put the changes into a separate commit to be squashed in the final version, if that works.
The updated (simpler) updated_default_commands approach @timabbott hinted at leaves the functions (were lambdas) pointing at the old data (notably for help & commands), so listing them in commands actually shows them twice (old default_commands and bot-provided commands list). This could be solved by going back to one data structure, if that could work, or some other approach involving passing in default_commands (new version) to the functions? (the alternative solution would be to just make default commands be all-or-nothing, ie. default_commands_enabled = False and commands is ignored, or True and they're used?)

roberthoenig · 2017-09-25T08:23:04Z

@neiljp I find this rather hard to review - it'd be awesome if you could split this in multiple, easily digestible commits; e.g. one for updating a bot with the feature, one for adding default commands, one for the core update command logic, etc.

Additionally, I think this would really profit from some unittests, which could for example be added in the updated bots themselves.

zulipbot · 2017-09-30T06:01:15Z

Hello @neiljp, it seems like you have referenced an issue in your pull request, but you have not referenced that issue in your commit message(s). When you reference an issue in a commit message, it automatically closes the corresponding issue when the commit is merged.

Please run git commit --amend in your command line client to amend your commit message description with "Fixes #[issue number]”.

An example of a correctly-formatted commit:

commit fabd5e450374c8dde65ec35f02140383940fe146
Author: zulipbot
Date:   Sat Mar 18 13:42:40 2017 -0700

    pull requests: Check PR commits reference when issue is referenced.

    Fixes #51

Thank you for your contributions to Zulip!

zulipbot · 2017-09-30T20:28:27Z

Hello @neiljp, it seems like you have referenced an issue in your pull request, but you have not referenced that issue in your commit message(s). When you reference an issue in a commit message, it automatically closes the corresponding issue when the commit is merged.

Please run git commit --amend in your command line client to amend your commit message description with "Fixes #[issue number]”.

An example of a correctly-formatted commit:

commit fabd5e450374c8dde65ec35f02140383940fe146
Author: zulipbot
Date:   Sat Mar 18 13:42:40 2017 -0700

    pull requests: Check PR commits reference when issue is referenced.

    Fixes #51

Thank you for your contributions to Zulip!

zulipbot · 2017-09-30T20:41:07Z

Hello @neiljp, it seems like you have referenced an issue in your pull request, but you have not referenced that issue in your commit message(s). When you reference an issue in a commit message, it automatically closes the corresponding issue when the commit is merged.

Please run git commit --amend in your command line client to amend your commit message description with "Fixes #[issue number]”.

An example of a correctly-formatted commit:

commit fabd5e450374c8dde65ec35f02140383940fe146
Author: zulipbot
Date:   Sat Mar 18 13:42:40 2017 -0700

    pull requests: Check PR commits reference when issue is referenced.

    Fixes #51

Thank you for your contributions to Zulip!

neiljp · 2017-09-30T20:47:12Z

I've refactored some of these commits to be smaller and clearer, and added some which amend the testing framework and zulip_bot_output.py to use the same logic as lib.py (before those commits, they provide different results from tests and running them, which indicates they should be in the same commit?).

I still would like to think/discuss about the way commands might work, but this is getting closer, I feel.

neiljp · 2017-09-30T21:05:46Z

Currently the CI is failing, since I'm unsure whether to encode the complex default command responses in the tests, when they're so automated from the other data? That is, should I update the "help" text for the xkcd bot to ensure it passes, or remove that test?

rht · 2017-10-01T05:11:37Z

zulip_bots/zulip_bots/lib.py

@@ -168,6 +168,14 @@ def is_private_message_from_another_user(message_dict, current_user_id):
        return current_user_id != message_dict['sender_id']
    return False

+def get_bot_details(bot_class, bot_name):


LGTm for the commit ffc977a

rht · 2017-10-01T05:30:18Z

zulip_bots/zulip_bots/lib.py

@@ -168,10 +170,30 @@ def is_private_message_from_another_user(message_dict, current_user_id):
        return current_user_id != message_dict['sender_id']
    return False

+def setup_default_commands(bot_details, message_handler):


I suppose this is the nucleus of the PR.

zulipbot · 2017-10-08T10:57:06Z

Hello @neiljp, a Zulip maintainer reviewed this pull request over 7 days ago, but you haven't updated your pull request since. Please take a look at the requested changes and update your pull request accordingly.

Thank you for your valuable contributions to Zulip!

showell · 2017-11-06T22:18:10Z

Hi @neiljp Can you resolve the conflicts here?

- empty: solves common case where "" message not handled in bots - about: uses new name/description META information from bots - usage: re-uses existing bot usage() function (remove/adjust later?)

zulipbot · 2017-11-29T18:08:53Z

Heads up @neiljp, we just merged some commits that conflict with the changes your made in this pull request! You can review this repository's recent commits to see where the conflicts occur. Please rebase your feature branch against the upstream/master branch and resolve your pull request's merge conflicts accordingly.

PIG208 · 2021-06-14T13:32:46Z

The concepts here are pretty valuable, but the PR has become too stale. I think we can close it but it's definitely worth doing a follow-up on this.

neiljp · 2021-06-15T07:52:59Z

@PIG208 This PR is very old now and out of date, but my recollection was of various driving elements:

Using the META tags to be able to not just know the user-name for a bot, but what bot code is running (if there is) behind it - by whatever method; some bots already have these added
Having a layer on top of the bots to make a standard set of commands, or at least simple ways of interacting with bots, easier; so eg. help, about, ... and these could use the META tags (or not)

neiljp · 2021-06-15T20:38:28Z

Note that #169 takes forward one of these points forward.

zulipbot added the needs review label Aug 15, 2017

neiljp mentioned this pull request Aug 15, 2017

Handle empty messages in bot framework by default #58

Closed

roberthoenig reviewed Aug 16, 2017

View reviewed changes

zulipbot added reviewed and removed needs review labels Aug 16, 2017

neiljp force-pushed the default_commands branch from fff413c to 6de4f99 Compare August 16, 2017 16:59

zulipbot added needs review and removed reviewed labels Aug 16, 2017

neiljp force-pushed the default_commands branch from 6de4f99 to db9934b Compare August 16, 2017 21:04

timabbott reviewed Aug 17, 2017

View reviewed changes

zulipbot added reviewed and removed needs review labels Aug 17, 2017

timabbott reviewed Aug 17, 2017

View reviewed changes

neiljp force-pushed the default_commands branch 2 times, most recently from ccb9841 to b775c48 Compare August 23, 2017 01:18

zulipbot added needs review and removed reviewed labels Aug 23, 2017

neiljp mentioned this pull request Aug 30, 2017

Handle empty bot messages through the bots framework. #55

Closed

neiljp mentioned this pull request Sep 7, 2017

[WIP: bots] Framework for simplified/reliable bot use/creation zulip/zulip#5047

Closed

neiljp force-pushed the default_commands branch from b775c48 to 9149ca9 Compare September 25, 2017 21:34

neiljp force-pushed the default_commands branch from 7f2d224 to 7926511 Compare September 30, 2017 20:28

neiljp force-pushed the default_commands branch from 7926511 to d0cc55c Compare September 30, 2017 20:41

neiljp force-pushed the default_commands branch from d0cc55c to 4e9f101 Compare September 30, 2017 21:02

rht reviewed Oct 1, 2017

View reviewed changes

zulipbot added reviewed and removed needs review labels Oct 1, 2017

rht reviewed Oct 1, 2017

View reviewed changes

showell force-pushed the master branch from 105db19 to 08e2121 Compare October 21, 2017 00:34

neiljp added 6 commits November 8, 2017 19:34

bots: Refactor bot_details handling into function.

9ab2312

bots: Add (optional) minimal default commands (empty, about, usage).

18d7df9

- empty: solves common case where "" message not handled in bots - about: uses new name/description META information from bots - usage: re-uses existing bot usage() function (remove/adjust later?)

bots: Refactor updating default commands into updated_default_commands.

16544d5

bot testing: Support testing of default commands.

97579be

bots: Enable zulip_bot_output.py to understand default commands.

5d4e5b4

bots: Add command-based default commands.

34b5f3a

neiljp force-pushed the default_commands branch from 4e9f101 to 34b5f3a Compare November 9, 2017 04:06

zulipbot added needs review and removed reviewed labels Nov 9, 2017

neiljp mentioned this pull request Nov 25, 2017

Bots: Add default (and other) command dispatch #169

Open

zulipbot added the has conflicts label Aug 9, 2018

Uh oh!

Bots: Add metadata scheme for bots. #66

Are you sure you want to change the base?

Bots: Add metadata scheme for bots. #66

Uh oh!

Conversation

neiljp commented Aug 15, 2017 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

roberthoenig left a comment • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

neiljp commented Aug 16, 2017

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

neiljp commented Aug 23, 2017

Uh oh!

zulipbot commented Aug 23, 2017

Uh oh!

neiljp commented Aug 27, 2017

Uh oh!

roberthoenig commented Sep 25, 2017

Uh oh!

zulipbot commented Sep 30, 2017

Uh oh!

zulipbot commented Sep 30, 2017

neiljp commented Aug 15, 2017 •

edited

Loading

roberthoenig left a comment •

edited

Loading

rht Oct 1, 2017 •

edited

Loading