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

zulip_bots: Add a script for creating Zulip bots. #709

Open
wants to merge 2 commits into
base: main
Choose a base branch
from

Conversation

PIG208
Copy link
Member

@PIG208 PIG208 commented Jul 30, 2021

Following support to running bots from entry points in #708, we
implement this create-zulip-bot tool to simplify the process of
creating new bots. The user will be able to directly install the package
with pip and run the bot with zulip-run-bot, or use it to quickly set
up a git repository.

Note that the boilerplate generated by this script does not contain
tests.py yet. We need to figure out the right pattern for integrating
unittests for such packaged bots.

"name": "{bot_name}",
"version": {bot_name}.__version__,
"entry_points": {{
"zulip_bots.registry": ["{bot_name}={bot_name}.{bot_name}"],
Copy link
Contributor

Choose a reason for hiding this comment

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

Does this working assume the generating directory (default .) is actually equal to the bot name that is supplied? If so, it seems cleaner to make the base folder and the nested one relative to the current location?

Copy link
Member Author

Choose a reason for hiding this comment

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

The name of the base directory doesn't matter. We assume that the nested directory and the bot module have the same name as the bot. This is the same practice we have for bots under the zulip_bots/zulip_bots/bots directory.

Copy link
Contributor

Choose a reason for hiding this comment

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

Right you are, this was from a quick read rather than test.

PIG208 added 2 commits August 3, 2021 01:31
This removes the need to use -r flag and instead load the bot from the
registry when no other method works.
Following support to running bots from entry points in zulip#708, we
implement this `create-zulip-bot` tool to simplify the process of
creating new bots. The user will be able to directly install the package
with pip and run the bot with `zulip-run-bot`, or use it to quickly set
up a git repository.

Note that the boilerplate generated by this script does not contain
`tests.py` yet. We need to figure out the right pattern for integrating
unittests for such packaged bots.
@timabbott
Copy link
Member

Can you explain the motivation for the first commit? I can imagine a world where one would prefer to find a bot using a specific path, and don't have a clear idea of whether there's any potential situation where we might prefer semantics of "Please only use a bot from the registry". Maybe the right thing to do is add a docstring in the bot discovery function explaining clearly the sources it looks at the motivation for why they're prioritized the way they are? That seems like a good resource in general and in particular would make it a lot easier for me to review this change.

@neiljp
Copy link
Contributor

neiljp commented May 12, 2023

@PIG208 This looks like it needs a rebase and tidy, but otherwise would progressing this be easier if the two commits were split between PRs? The first commit isn't necessary for the second, right?

bot_module_path = Path(bot_path, args.bot)

try:
os.mkdir(bot_path)
Copy link
Contributor

Choose a reason for hiding this comment

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

Reading this again, is there a reason not to use the pathlib functions instead, if we're already using Path? That is, can we drop import os?

@zulipbot
Copy link
Member

Heads up @PIG208, we just merged some commits that conflict with the changes you 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/main branch and resolve your pull request's merge conflicts accordingly.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants