Skip to content

Conversation

@onerandomusername
Copy link
Contributor

@onerandomusername onerandomusername commented Oct 21, 2025

Update botstrap to be much more userfriendly. This silences noisy logs that we don't want to see or care about when downloading new server.

We also provide a success message when the entire system is successful.

I've also implemented some small caching in the botcore client to only make requests when necessary, and to only make one request to get all of the webhooks. While yes, webhooks are free, I left request logging enabled so the user could see what requests were made. I didn't see a good reason to silence those, both for transparency and because they already provide logging.

We also provide an invite link if the bot isn't in the guild already, but it is configured.

I've also added support which sets the app's flags to enable message content intents and member intent if they aren't already enabled.

closes #3414

Copy link
Member

@jb3 jb3 left a comment

Choose a reason for hiding this comment

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

Few review comments.

Can we also squash down the history a little bit? I don't think we need 21 commits for the changes here. Obviously keep the constants one separate but I think we can squash down a lot of the other ones.

@onerandomusername onerandomusername force-pushed the qt/beginner-friendly-botstrap branch 5 times, most recently from f5f2c3c to a61eb26 Compare October 25, 2025 21:33
@onerandomusername onerandomusername requested a review from jb3 October 25, 2025 21:37
@onerandomusername
Copy link
Contributor Author

@jb3 I've compressed the history and addressed your comments in the updated history. Comments should now follow pep8, formatted with ruff, and silenced pyright.

Copy link
Member

@jb3 jb3 left a comment

Choose a reason for hiding this comment

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

Thanks for addressing review feedback.

Only a couple of points left now, but also a couple of wider points:

  • I think the with block we use now to run the script is too long and I think things like the webhook management & emoji management should be moved into their own methods that we then call (the with should be as small as possible really).
  • The existing webhook if statement is still a bit scary (I thought I'd left a review comment but can't see/find it now!), I'd prefer creating a new method along the lines of find_matching_webhook, something like:
def find_matching_webhook(existing_webhooks, webhook_model, formatted_name, channel_id):
    for hook in existing_webhooks:
        if hook["id"] == str(webhook_model.id):
            return hook["id"]
        if hook["name"] == formatted_name and hook["channel_id"] == str(channel_id):
            return hook["id"]
    return None

It's all just become a little bit too complex to follow under that context manager and I think that some splitting up into smaller more digestible methods is a good idea here.

Copy link
Member

@jb3 jb3 left a comment

Choose a reason for hiding this comment

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

Sorry for these comments all being in parts, just keep doing scans and picking up new bits.

Can we also standardise on whether you're using f-strings in this file or using %s interpolation? There is a mix currently (I appreciate it was f-strings before which is not correct, but if you're adding some f-strings and some %s strings we may as well unify).

- provide completion message
- silence httpcore logger
- filter out botcore's warning for patching send_typing
- use short url for contributing link
- only use logging.warning and up for things that need user action
- use logging.fatal when exiting due to error
- use % for all logging statements
@onerandomusername onerandomusername force-pushed the qt/beginner-friendly-botstrap branch from a61eb26 to 57c92b0 Compare October 27, 2025 14:38
this lowers the amount of requests we make and reduces the chance of ratelimiting
* request all webhooks in a single call rather than per configured webhook

* Enable support for multiple identical test bots
sharing the same webhook

This should prevent the limit of 10 webhooks per channel from being reached
without needing to share a specific guild configuration for the PyDis
staff test guild
this makes it easier to configure webhooks:

we now only need to set their ID and not their channel.

Setting the channel ID is only use for botstrap configuration, anyhow.
@onerandomusername onerandomusername force-pushed the qt/beginner-friendly-botstrap branch from 57c92b0 to 82fa0b1 Compare October 27, 2025 19:37
botstrap.py Outdated
Comment on lines 397 to 407
if (
# check the existing ID matches the configured one
existing_hook["id"] == str(webhook_model.id)
or (
# check if the name and the channel ID match the configured ones
existing_hook["name"] == formatted_webhook_name
and existing_hook["channel_id"] == str(all_channels[webhook_name])
)
):
webhook_id = existing_hook["id"]
break
Copy link
Member

Choose a reason for hiding this comment

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

I have commented every review this needs changing 🥲.

Split out to a helper method without the complex conditions. Also, comments should always have a capital (and only be present if they actually add something, for simple conditionals they generally don't).

def find_matching_webhook(existing_webhooks, webhook_model, formatted_name, channel_id):
    for hook in existing_webhooks:
        if hook["id"] == str(webhook_model.id):
            return hook["id"]
        if hook["name"] == formatted_name and hook["channel_id"] == str(channel_id):
            return hook["id"]
    return None

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I sort of cleaned up this code but I'm not sure about the separate method with just how many specific inputs are required.

botstrap.py Outdated
Comment on lines 467 to 468
with botstrap:
botstrap.run()
Copy link
Member

Choose a reason for hiding this comment

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

I'm not sure if this is the way this should be done or whether it'd be cleaner to create a DiscordClient here, pass it into the botstrap initialiser and then avoid having to define the __exit__ method for the BotStrapper class.

e.g.

with DiscordClient(guild_id=GUILD_ID) as discord_client:
    botstrap = BotStrapper(client=discord_client)

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I would prefer to manage the DiscordClient internally on the botstrap class, even if we use contextlib to manage the context manager.

DiscordClient should not be used by anything other than the BotStrap class, and an instance of it outside of botstrap doesn't need to exist.

self.client.__exit__(exc_type, exc_value, traceback)

config_str += "\n#Categories\n"
def upgrade_client(self) -> bool:
Copy link
Member

Choose a reason for hiding this comment

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

No need to return a value here if you never consume it.

botstrap.py Outdated
def create_forum_channel(self, channel_name_: str, category_id_: int | str | None = None) -> str:
"""Creates a new forum channel."""
payload = {"name": channel_name_, "type": GUILD_FORUM_TYPE}
payload: dict[str, Any] = {"name": channel_name_, "type": GUILD_FORUM_TYPE}
Copy link
Member

Choose a reason for hiding this comment

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

nitpick: dict[str, object] (avoid Any unless really necessary)

def _raise_for_status(response: Response) -> None:
response.raise_for_status()

@property
Copy link
Member

Choose a reason for hiding this comment

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

@property should never be doing an HTTP request IMO. Maybe change these to methods?

Copy link
Member

Choose a reason for hiding this comment

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

Maybe a good case for a switch to a method with https://docs.python.org/3/library/functools.html#functools.cache

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.

More user friendly botstrap

3 participants