-
-
Couldn't load subscription status.
- Fork 731
refactor: update botstrap to be more beginner-friendly #3417
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
base: main
Are you sure you want to change the base?
refactor: update botstrap to be more beginner-friendly #3417
Conversation
496b553 to
a0b56a6
Compare
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.
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.
botstrap.py
Outdated
| response = self.post( | ||
| f"/guilds/{self.guild_id}/channels", | ||
| json=payload, | ||
| headers={"X-Audit-Log-Reason": f"PyDis Botstrap: Creating {channel_name_} forum channel"}, |
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.
Redundant to use f-string when this is always going to be alongside the channel name when shown in Discord.
| headers={"X-Audit-Log-Reason": f"PyDis Botstrap: Creating {channel_name_} forum channel"}, | |
| headers={"X-Audit-Log-Reason": "Creating forum channel as part of PyDis botstrap"}, |
botstrap.py
Outdated
| """A boolean that indicates if a channel is of type GUILD_FORUM.""" | ||
| response = self.get(f"/channels/{channel_id_}") | ||
| return response.json()["type"] == GUILD_FORUM_TYPE | ||
| return next(filter(lambda c: c["id"] == channel_id_, self.guild_channels)).get("type", None) == GUILD_FORUM_TYPE |
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.
This isn't very clean and channel type is a mandatory attribute from the API, split it up and remove the unnecessary get() usage.
I suggest a new helper method to return a channel from self.guild_channels and then this method just has to use that and check the type key.
I'd also remove the unnecessary _ suffixes from the arguments to these helper methods.
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.
To be fair, given the template now has the forum channel in it, this could be removed entirely. In my testing the code that upgrades python-help doesn't run when a guild is created from the template.
ada96ef to
deed1ff
Compare
- 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
e624361 to
f5f2c3c
Compare
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.
f5f2c3c to
a61eb26
Compare
|
@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. |
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.
Thanks for addressing review feedback.
Only a couple of points left now, but also a couple of wider points:
- I think the
withblock 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 NoneIt'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.
|
|
||
| def get_emoji_contents(self, emoji_id: str | int) -> bytes | None: | ||
| """Fetches the image data for an emoji by ID.""" | ||
| # emojis are located at https://cdn.discordapp.com/emojis/821611231663751168.png?size=4096 |
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.
I'd use path-params to document this and where that snowflake comes from (even though it's obvious). Also, not sure the size is necessary given we don't then use it for the subsequent request.
| # emojis are located at https://cdn.discordapp.com/emojis/821611231663751168.png?size=4096 | |
| # Emojis are located at https://cdn.discordapp.com/emojis/{emoji_id}.png |
| server_channels = self.guild_channels | ||
|
|
||
| for channel in server_channels: |
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.
| server_channels = self.guild_channels | |
| for channel in server_channels: | |
| for channel in self.guild_channels: |
| """Check if the bot is a member of the guild.""" | ||
| try: | ||
| _ = self.guild_info | ||
| except HTTPStatusError: |
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.
Maybe we can also check the status here? Avoids us having problems from a transient Discord error that we should surface in a different way.
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.
We don't check these errors as it is, we just fail hard on them. Good point regarding our raise_for_status hook. We should maybe try again if it's 5xx and fail on 4xx.
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.
Yeah it could go in the hook, I just think specifically with this case you're silencing server errors. The current behaviour elsewhere being failing hard is fine but this is not failing hard, this is all 4XX and 5XX being treated the same and silenced.
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.
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).
| def get_channel(self, id: int | str) -> dict[str, Any]: | ||
| """Fetches a channel by its ID.""" | ||
| for channel in self.guild_channels: | ||
| if channel["id"] == str(id): | ||
| return channel | ||
| raise KeyError(f"Channel with ID {id} not found.") | ||
| @property |
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.
Missing a newline after this & the id argument is shadowing id().
| response = self.get(f"{self.CDN_BASE_URL}/emojis/{emoji_id!s}.webp") | ||
| return response.content | ||
|
|
||
| def clone_emoji(self, *, name: str, id: str | int) -> str: |
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.
id argument shadowing id().
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