#dev-log

1 messages ยท Page 72 of 1

regal archBOT
odd spireBOT
oak estuaryBOT
regal archBOT
odd spireBOT
regal archBOT
odd spireBOT
regal archBOT
#
[python-discord/site] New branch created: content\-migration
#

This PR migrates pages under https://pythondiscord.com/pages/ to the site repo as part of dewikification.

How to Review

  1. Run the branch locally.
  2. Compare each page with the corresponding page on the current website.
  3. Don't suggest grammar/phrasing changes for any existing page content, however new content like category and page descriptions can be brought up for improvement.
  4. Check if links function as intended, and that any images used are local static images and not hotli...
regal archBOT
regal archBOT
#
[python-discord/site] New branch created: ks129/dewikification/redirection
odd spireBOT
odd spireBOT
regal archBOT
odd spireBOT
regal archBOT
#

Relevant Issues

No relevant issues as this was discussed in #dev-contrib.

Description

Instead of using discord.Emoji for the .emoji, I changed it to discord.PartialEmoji as the PartialEmoji has all the attributes the .emoji command uses.

Reasoning

The current .emoji command doesn't currently allow emojis that the bot cannot see, and this PR changes so that any custom emoji can be used with this command.

Screenshots

Before:
![before-image](ht...

odd spireBOT
regal archBOT
odd spireBOT
regal archBOT
#
[python-discord/bot] New branch created: mbaruh/allow\_eval
#

Description

Adds a check to blacklist a command only in a specific context, with an option for a role override. The check is applied to the eval command to blacklist it only from python-general.

Questionable choices

  • The check uses the in_whitelist_check check internally. It's possible that it's better to manually handle the logic of the predicate.
  • This still uses the InWhiteListCheckFailure exception, even though it explicitly states in its docstring that it's raised for the i...
odd spireBOT
regal archBOT
odd spireBOT
regal archBOT
#

d14f83a add custom command checks tag - vcokltfre
029f4aa update wording to emphasise checks not decorators - vcokltfre
fddfc76 rename function to in_any_channel in accordance... - vcokltfre
15aa872 chore: update wording as requested - vcokltfre
132c985 Merge branch 'main' into decorator-tag - Xithrius

#
[python-discord/bot] branch deleted: decorator\-tag
oak estuaryBOT
odd spireBOT
regal archBOT
regal archBOT
#

closes: #1502

Implements the BOT_TRACE_LOGGERS env var that can be used to set which loggers should log trace logs instead of defaulting to logging them all in a development environment.

The env var can be either * or ROOT to set the root logger (and with it all other loggers) to the trace level, or a string in the format of logger_name,another.logger_name to set specific loggers to the level.

I removed the option to use the COLOREDLOGS_LOG_LEVEL to change the coloredlogs l...

odd spireBOT
regal archBOT
odd spireBOT
regal archBOT
regal archBOT
regal archBOT
odd spireBOT
regal archBOT
#

What is WebP compat like across browsers?

No IE, everything else within reason fully supports, Safari on >= MacOS 11 Big Sur.
I primarily went with WebP as it's 3x smaller than PNG.

Editors tend to strip trailing whitespace. Is there an alternative to using trailing whitespaces?

I'll look into <br> tags, they are few and far between so it's worth the markup tradeoff to deal with the editor issue.

regal archBOT
odd spireBOT
odd spireBOT
regal archBOT
odd spireBOT
regal archBOT
odd spireBOT
regal archBOT
#

Description

The PR template has a lot going for it. I like the fields it has, and the descriptions written there for each one are quite adequate, yet, we still end up with a lot of PRs that either doesn't use the template and fail to provide information in any other way, or fail to fill in on or more fields that contain very important information. This applies to staff, core-devs, and non-staff contributors.

I won't list specific examples here as to not call anyone out, but I have se...

odd spireBOT
regal archBOT
#

Preliminary block on this PR (I have not reviewed the actual content yet). This PR can not be merged yet, for the same reason our other projects can not be migrated. A lot of the dependencies you've bumped do not have wheels for 3.9 yet, which means these changes are not acceptable through no fault of your own. Without wheels, you need build tools, which significantly hurts people contributing on systems that don't have them by default (such as windows), and will not work in our CI. You can t...

odd spireBOT
regal archBOT
#

This PR adds a large amount of code style changes that I cannot really wrap my head around. I see you decided to run Black on the code, do you think the lines that got broken were too long? New logging is inconsistent in style with both itself and the existing code; some existing logging was changed but not all.

None of the documentation, class or function, was updated to mention the new behaviour.

I'm still not convinced that we need to support more than 10 embeds per report. Such an a...

regal archBOT
regal archBOT
#
[python-discord/sir-lancebot] New branch created: remove\-ffmpg
#

Description

Removed apt get update and ffmpg install as it's no longer needed.
Removed the voice extra for d.py and deleted the un-used sound clips.

Draft until fully tested.

Reasoning

The sound clips, and dependencies were no longer bring used by any command, so fulfilled no purpose in the bot.
Removing these will speed up build times

Did you:

odd spireBOT
regal archBOT
#

do you think the lines that got broken were too long?

Not exactly sure, if they were too long or if black decided to break them.

New logging is inconsistent in style with both itself and the existing code; some existing logging was changed but not all.

Could you point them out?

None of the documentation, class or function, was updated to mention the new behaviour.

I completely forgot about them, will have them updated.

I'm still not convinced that we need to suppo...

regal archBOT
regal archBOT
regal archBOT
odd spireBOT
regal archBOT
odd spireBOT
regal archBOT
night lilyBOT
#
Sir Lancebot

Connected!

odd spireBOT
regal archBOT
#

Pull requests need to have related issues. Opening a pull request without an issue requires an explicit core-dev approval beforehand.

Could be shorter

Pull requests must have a corresponding issue unless explicitly given permission from a core dev.

Corresponding issue denote a stronger relationship than related issue, though maybe the word is too "fancy". Must has a stronger connotation than needs to.

#

I agree with shortening that part. I think mentioning core dev approval in there is important, what do you think of:

Opening a pull request without an issue requires explicit core dev approval.
(mostly dropping the first sentence)

I've rewritten that whole section to be more to the point (implicitly addresses your second comment):
Original

This field is mandatory. List relevant issue tickets here.
All changes require explicit Core Developer approval (for issues, those are given us...

#

Opening a pull request without an issue requires an explicit core-dev approval beforehand.

I think it would be nice to add that if it was discussed in Discord and you were told not to open an issue, you must provide links to the conversation.

It's not uncommon that admins and owners say that you can open a pr, so I think that making sure you provide links is something worth pointing out.

regal archBOT
regal archBOT
odd spireBOT
regal archBOT
oak estuaryBOT
odd spireBOT
regal archBOT
odd spireBOT
regal archBOT
regal archBOT
regal archBOT
regal archBOT
#

I might not be following. Even if you execute all filters in the right order for a specific message, if it is deleted by one of them, it is possible that in a few seconds it will turn out it's part of a larger stream of spam messages that should be punished. At that point, the message is already deleted and will therefore won't be taken into account.

#

I might not be following. Even if you execute all filters in the right order for a specific message, if it is deleted by one of them, it is possible that in a few seconds it will turn out it's part of a larger stream of spam messages that should be punished. At that point, the message is already deleted and therefore won't be taken into account.

You're right; I was just thinking about it the wrong way.

regal archBOT
odd spireBOT
regal archBOT
#

Thanks for the PR! I have a few comments to do:

  • Please revert lines broken down by Black. You have free to use formatters on your contributions but they must comply with our code style. We don't use 79 chars limits.
  • We decided to not allow more than 10 embeds per report. This can reduce code complexity.
  • Entries from the cache should be deleted after the incident is cleared. We shouldn't keep the cache growing exponentially.
  • Message edits aren't handled properly, they should. Also ...
regal archBOT
odd spireBOT
regal archBOT
odd spireBOT
regal archBOT
#

This issue documents the planned changes to the filters extension. Minor details may still change, and there will be an accompanying site issue. This issue specifies the full extent of the desired changes, but this can be split into several issues and made into a project.

The changes allow for granular control over the behavior of individual filters, and increase the scope of it. All aspects of the extension, including automatic rules, will be controlled from a single entry-point.

Fil...

regal archBOT
#

Abstract

While https://github.com/python-discord/bot/issues/1530 describe the new changes we want for the filter cog group in our bot functionality-wise, this issue will focus on the new database schema proposed to implement those changes. I'd highly recommend you to check out 1530 before reading this issue.

New Schema Overview

Here is a visual representation of the proposed schema :
![image](https://user-images.githubusercontent.com/44778521/115118668-9f3d9c00-9fa4-11eb-8a6b...

odd spireBOT
night lilyBOT
#
Sir Lancebot

Connected!

odd spireBOT
regal archBOT
#
[python-discord/sir-lancebot] branch deleted: reject\-dms\-default\-check
regal archBOT
odd spireBOT
regal archBOT
odd spireBOT
regal archBOT
odd spireBOT
odd spireBOT
regal archBOT
#

It'd be helpful for the following pages to have a table of contents:

  • FAQ
  • Code Reviews: A Primer
  • Function Parameters and Arguments in Python
  • Helping Others
  • Help Channels
  • Asking Good Questions
  • Working with Docker & Docker Compose
  • Preparing Your Hosts file

Thoughts?


None of the "more" links in the navbar work. Will that be addressed later?


The compression artefacts on many images are fairly noticeable. Can you use higher-quality im...

regal archBOT
oak estuaryBOT
#

Doc item doc_item.symbol_id='assert' present in loaded documentation inventories not found on site, inventories may need to be refreshed.

#

Doc item doc_item.symbol_id='fixtures' present in loaded documentation inventories not found on site, inventories may need to be refreshed.

#

Doc item doc_item.symbol_id='module-pytest' present in loaded documentation inventories not found on site, inventories may need to be refreshed.

odd spireBOT
regal archBOT
regal archBOT
#

Looks & works great. I agree that having trace logs enabled globally by default is annoying, so this will be a great change. Let me share some thoughts.

Have you considered defining the config directly in the YAML? It would allow us to use the list structure, rather than a comma-delimited string. The local config is already git-ignored, so I don't think we would lose out on the convenience there.

It may even allow us to drop support for the * and ROOT aliases since having an empty strin...

regal archBOT
regal archBOT
regal archBOT
regal archBOT
regal archBOT
#

Have you considered defining the config directly in the YAML? It would allow us to use the list structure, rather than a comma-delimited string. The local config is already git-ignored, so I don't think we would lose out on the convenience there. I think this would also allow us to make it a list always (empty by default), rather than consider None.

I considered it at first, but saw the env var as something that's easier to change for the user with the config being mostly static

It ...

night lilyBOT
#
Sir Lancebot

Connected!

regal archBOT
#

I think there may be some merit in allowing the root aliases to be part of the list, rather than the sole entry. It seems unexpected that a,a.b would work to enable a.c, bot not *,a,a.b to enable c.

I actually think it would be a good opportunity to implement a blacklist. That is, if * is present, then all other specified loggers have trace disabled. If *, then all specified loggers have trace enabled. A blacklist works well with my flow since there are only a couple modules that ha...

odd spireBOT
night lilyBOT
#
Sir Lancebot

Connected!

odd spireBOT
regal archBOT
#

I actually think it would be a good opportunity to implement a blacklist. That is, if * is present, then all other specified loggers have trace disabled. If *, then all specified loggers have trace enabled. A blacklist works well with my flow since there are only a couple modules that have annoying trace logs; the rest don't bother me most of the time. A blacklist would mean I don't have to change the value of the env var every time I test a different feature.

That also sounds useful...

#

I made a typo. I meant that if * is not present, then specified loggers will be set to trace. To enable it globally all you'd need is a lone *.

Yep, but to enable it, the old logger names would be lost from the env var which could be annoying when doing it temporarily.

So we'd have something like
a - enables for module a
*,a or *a - enables for everything
!,a or !,a - disables for a

odd spireBOT
odd spireBOT
regal archBOT
#

Do the filters apply only in specific channels / categories

What about filtering by roles? The obvious use case is to exclude staff members from some filters.

Reaction Menus

While I'm not the target user for this feature, I'm not sold on the necessity of it.

it should be explored whether it's possible to present in the alert all tokens which triggered unique filters (meaning, we don't have to show all tokens that triggered a specific filter).

Can you rephrase this? I don'...

odd spireBOT
regal archBOT
odd spireBOT
regal archBOT
#

When using the !docs command to see a list of all of the available packages, any package that starts with a capital letter seems to be brought to the top of the list. Currently the only package starting with a capital letter is GitPython

Image of returned embed when !docs is used:
image

I would be happy to fix this myself.

regal archBOT
odd spireBOT
regal archBOT
regal archBOT
regal archBOT
#

This problem exists for me on Windows 10 Pro 20H2 using the following browsers:

  • Vivaldi: 3.7.2218.45 (Stable channel) (64-bit)
  • Brave: Version 1.23.71 Chromium: 90.0.4430.72 (Official Build) (64-bit)
  • Google Chrome: Version 92.0.4482.0 (Official Build) canary (64-bit)
  • Google Chrome: Version 89.0.4389.128 (Official Build) (64-bit)
  • Opera: Version:75.0.3969.171
  • Edge: Version 89.0.774.77 (Official build) (64-bit)
  • Firefox: 87.0 (64-bit)
#

Well may I explain myself then.

  1. I get the width and the height of the Image passed to the function.

  2. I get the root of a number of squares passed which I call xy. Reason: if let's say 25 squares were passed, that is the total squares (split pieces) that the image is supposed to be split into. As you know, a 2D shape has a height and a width, and for this case I think of it as rows and columns. Rows multiplied by columns is equal to the passed squares. To get rows and columns, since...

regal archBOT
odd spireBOT
regal archBOT
regal archBOT
#

Here it is.

  1. I shuffle passed imgs. Reason: to randomize the squares (split pieces).

  2. I get a single square (split piece) out of the list and define single_width as the square's width and single_height as the square's height.

  3. I get the root of type integer of the number of imgs (split pieces) in the list and call it multiplier. I then proceed to calculate total height and width of the new image that I am creating using the same multiplier.

  4. I then define new_img as the im...

odd spireBOT
regal archBOT
regal archBOT
regal archBOT
regal archBOT
odd spireBOT
regal archBOT
regal archBOT
#

Description

The !zen command has support for negative indexing. This means that since there are 18 lines of the zen of python, doing !zen -18 should give the first line (0th index). However, this is not the case; doing !zen -18 shows the second line (1st index). The problem seems to be this line: https://github.com/python-discord/bot/blob/main/bot/exts/utils/utils.py#L112

Screenshots

![image](https://user-images.githubusercontent.com/78174417/115246235-b2ab4d00-a0f3-11eb-9b1f...

odd spireBOT
regal archBOT
odd spireBOT
regal archBOT
regal archBOT
#

When a user sets a reminder, an confirmation embed is sent, to inform the user that the reminder was successfully saved.

In this reminder, we currently include the time at which it is due to be sent. This time is in UTC.

I propose we refactor this part of the code to instead use the timestamp field in the Embed.
https://github.com/python-discord/bot/blob/f2970481e1cd8950dbbb2d3b871df68e90f5a932/bot/exts/utils/reminders.py#L270-L284
This would mean that the time would automatically g...

#

Description

We would like to see an addition to the Sir Lancebot commands: .catify. This would insert random แ“šแ˜แ—ข's into a message sent by the invoker of the command.

Reasoning

This would be useful since it enhances the แ“šแ˜แ—ข cult.

Proposed Implementation

Additional Details

Would you like to implement this yourself?

  • [ ] I'd like to implement this feature myself
  • [x ] Anyone can implement this feature
#

I would have moved this into the user's nickname instead, as this is were it is getting used mostly. For example, when i do .catify, my nickname should be changed to Shivansh-007 แ˜กแ˜แ—ข.

Another feature I would like to have is, a list of different cats, from which it would be randomly chosen and put into the user's nickname.

regal archBOT
regal archBOT
#
[python-discord/bot] New branch created: list\-non\-staff\-with\-stream\-perms
#
[python-discord/workers] New branch created: shipit
odd spireBOT
odd spireBOT
regal archBOT
night lilyBOT
#
Sir Lancebot

Connected!

odd spireBOT
regal archBOT
odd spireBOT
regal archBOT
#

Description

The ability to run .duck or .manduck command to get a random duck or manduck, generated by quackstack.

Reasoning

Quackstack is currently fairly inaccessible since it's pretty much just a raw API. One thing we can do to make it more accessible to everyone is to allow them to use it through lancebot to create their own duckies!

Proposed Implementation

Unsure of exact implementation, but roughly:

  • Make POST to quackstack to generate a random duck
  • Send t...
odd spireBOT
regal archBOT
#
[python-discord/bot] branch deleted: list\-non\-staff\-with\-stream\-perms
#

cb25375 Require a mod role for stream commands - ChrisLovering
90ed28f Add command to list users with streaming perms - ChrisLovering
a6b7609 Update wording of comment to be clearer. - ChrisLovering
94db90b Remove unnecessary _ in variable name - ChrisLovering
131dab3 Improve the wording of the list streamers embed - ChrisLovering

oak estuaryBOT
odd spireBOT
regal archBOT
odd spireBOT
regal archBOT
odd spireBOT
regal archBOT
#

What about filtering by roles? The obvious use case is to exclude staff members from some filters.

You're right; edited.

While I'm not the target user for this feature, I'm not sold on the necessity of it.

The commands are kinda long and hard to remember, so what ends up happening is that someone will use !bl, and then they see the available commands, and then they use say !bl list, because we don't remember what the names of the lists are, and only after seeing the list name...

odd spireBOT
regal archBOT
#

Missed this one

What kind of granularity is needed? Specific filter ID(a), filter list type(s), or all filters?

Whatever is needed to uniquely identify a filter. By how the site schema looks right now (not sure if Akarys updated it after opening the issue, we continued discussing it since), it seems that each filter will have a unique ID. So a filter ID - user ID's pairing should be enough.

odd spireBOT
regal archBOT
odd spireBOT
regal archBOT
odd spireBOT
regal archBOT
odd spireBOT
odd spireBOT
odd spireBOT
regal archBOT
odd spireBOT
odd spireBOT
regal archBOT
odd spireBOT
regal archBOT
odd spireBOT
regal archBOT
#

I'm wondering if it would be better to do the same thing done to set the default duration of the mute command, meaning use the Duration converter to create the default time. This would ensure that if there is ever a change to the implementation of the converter, the behaviors won't diverge.

I thought of doing that, but the Duration().converter(...) needs to parse and convert the string 1h to a timedelta object first and then add the date and timedelta object.
Whereas here, we can di...

odd spireBOT
regal archBOT
odd spireBOT
regal archBOT
#

The commands are kinda long and hard to remember

Fair enough. However, a menu seems clumsy to me, and the amount of code required to implement one of this scale seems non-trivial.

The issue is that it's not possible for the list of custom filters, because each such custom filter will have its own predicate, which can be arbitrarily complex. So I'd rather if those classes contained their predicates, and then load those filters dynamically from a module, instead of the filter list con...

#

I thought of doing that, but the Duration().converter(...) needs to parse and convert the string 1h to a timedelta object first and then add the date and timedelta object.
Whereas here, we can directly add the date and timedelta object

True, but I don't really mind the extra operation. It's cheap and hidden anyway.

alright then, we can use the Duration() class directly

odd spireBOT
regal archBOT
odd spireBOT
odd spireBOT
regal archBOT
#

Description

If someone forgets the ! or misspells a command and they edit their message to fix it, the bot should notice that edit and run that command normally - as if it was originally typed correctly. This would only work for messages that were edited within the past 30 seconds or so.

This could be done automatically upon edit or by adding a repeat emoji reaction ๐Ÿ” to the message that the user has to click when the bot recognizes the message is now a valid command. The latter is s...

oak estuaryBOT
odd spireBOT
regal archBOT
regal archBOT
#
jb3

Had a test locally, things mostly work although when a single word is passed (e.g. .catify hello) the command crashes out with:

04/19/21 22:00:45 - root DEBUG: Error Encountered: ValueError - empty range for randrange() (1, 1, 0), Command: catify, Author: joe#6000, Channel: bot-commands
04/19/21 22:00:45 - bot.exts.evergreen.error_handler ERROR: Unhandled command error: empty range for randrange() (1, 1, 0)
Traceback (most recent call last):
  File "/workspaces/sir-lancebot/.venv/l...
#
    @commands.command(aliases=["แ“šแ˜แ—ขify", "แ“šแ˜แ—ข"])
    async def catify(self, ctx: commands.Context, *string: Optional[str]) -> None:
        """Catifies a string or username."""
        if string == ():
            username = ctx.author.name
            if len(username) >= 28:
                return await ctx.send("Your username is too long to be catified! Please change it.")
            else:
                username += f" | {random.choice(Cats.cats)}"
                await ct...
#
    @commands.command(aliases=["แ“šแ˜แ—ขify", "แ“šแ˜แ—ข"])
    async def catify(self, ctx: commands.Context, *string: Optional[str]) -> None:
        """Catifies a string or username."""
        if string == ():
            username = ctx.author.name
            if len(username) >= 28:
                return await ctx.send("Your username is too long to be catified! Please change it.")
            else:
                username += f" | {random.choice(Cats.cats)}"
                await ct...
#
   @commands.command(aliases=["แ“šแ˜แ—ขify", "แ“šแ˜แ—ข"])
    async def catify(self, ctx: commands.Context, *string: Optional[str]) -> None:
        """Catifies a string or username."""
        if string == ():
            username = ctx.author.name
            if len(username) >= 28:
                return await ctx.send("Your username is too long to be catified! Please change it.")
            else:
                username += f" | {random.choice(Cats.cats)}"
                await ctx...
#

More code :D

   @commands.command(aliases=["แ“šแ˜แ—ขify", "แ“šแ˜แ—ข"])
    async def catify(self, ctx: commands.Context, *string: Optional[str]) -> None:
        """Catifies a string or username."""
        if len(" ".join(string)) > 2000 - 2000//3:
            embed = discord.Embed(title="your message was so immense it overloaded my computing core. no cats for you.", color=discord.Colour.red())
            await ctx.channel.send(embed=embed)
            return
        if string == ():
...
#

since the max cats is 1/3 of words

now that i think of it, the 2/3 doesn't make sense since 1/3 of words is... well, words, not letters. but i guess the words are separated by spaces and so i f y o u t y p e l e t t e r b y l e t t e r l i k e t h i s it will work... but then it also won't, bc spaces count as well towards limit... AHHHHHHHHHHHHHHHHHHH TO MANY STUFF TO THINK ABOUT

regal archBOT
odd spireBOT
regal archBOT
odd spireBOT
regal archBOT
odd spireBOT
regal archBOT
regal archBOT
regal archBOT
#

Description

With the current paginator, when a new image is set on a page, the paginator shows the same image in the next page if the next page does not have an image of its own

Current source code:

image = paginator.images[current_page]
if image:
    embed.set_image(url=image)

If the next page's image is an empty string, then the paginator uses the image from the last page.

Solutions

  1. Set image to empty string when required.
image = paginator.i...
odd spireBOT
odd spireBOT
regal archBOT
regal archBOT
#

In https://github.com/python-discord/organisation/issues/341 we discussed the want to store and process messages from help channels. This is so that we can better investigate the usages of our help channels, in order to improve them effectively.

Implementation suggestions (from Akarys) are:

  • Store content of messages posted in help channels in metricity
    • The whole infra is built around data coming from metricity, I think it is the most obvious choice. It won't require too much
      ...
regal archBOT
#
jb3

Dropped this internally, but here is my list of recommendations from a privacy standpoint:

  • opt-out system which erases on opt-out
  • controlled access to the dataset (admins + select few others)
  • holding for no longer than a month or two
  • no user data above a discord user ID
  • on message delete on the server we remove from our dataset, even if the user is opted in
  • data is held and collected by a separate service

I'm very against putting this information in Metricity since we mad...

odd spireBOT
regal archBOT
#

A couple comments/questions I had:

  • Will the preexisting metricity opt out users (if the list still exists) be opted out of this automatically?
  • If we made promises not to store messages in metricity, wouldnโ€™t it be a bit scummy to store them in what is effectively metricity, but under a different name.
  • Completely agree on not making the dataset public (side note: not even admins need access to be honest. I imagine youโ€™ll want to analyze those messages and extract some statistics whic...
#
jb3
  1. No, that list is gone as of Sunday 12UTC. The list was fairly small, and in fact the majority of users on the list had left by the time that it was discontinued on Sunday.

  2. I don't think it's scummy, this is just clarity. We've said before that Metricity won't store message contents and I think then later altering that so that it does collect it a bad idea, it's not just that it doesn't meet the goals of the project but that it's odd to integrate message collection into a service wher...

odd spireBOT
regal archBOT
regal archBOT
odd spireBOT
odd spireBOT
regal archBOT
regal archBOT
odd spireBOT
regal archBOT
#

@jb3 i think this is the good copy, would you mind looking at it and review? thank you very much!

quick summary on additions & bug fixes while you were offline, in chronological order:

  1. fixed one-word crash bug
  2. added 1500 input character cap
  3. change any elements in bot.constants.Cat.cats to "cat"
  4. docstring update, more spacing, whitespace cleaning

minor changes:

  1. "quality-of-life changes - from @Kronifer, not sure what it means
  2. changed displayname limit from < 26 ...
regal archBOT
#
[python-discord/bot] New branch created: test\-async\-mock
odd spireBOT
odd spireBOT
regal archBOT
odd spireBOT
odd spireBOT
odd spireBOT
regal archBOT
odd spireBOT
odd spireBOT
night lilyBOT
#
Sir Lancebot

Connected!

regal archBOT
odd spireBOT
regal archBOT
odd spireBOT
regal archBOT
#
[python-discord/bot] branch deleted: mbaruh/offduty
odd spireBOT
regal archBOT
oak estuaryBOT
odd spireBOT
night lilyBOT
#
Sir Lancebot

Connected!

#
Sir Lancebot

Connected!

regal archBOT
#

Description

I indented some code into the else statement because some variables that were defined in it were used outside of it, and suppressed discord.Forbidden when someone with a higher role than Sir Lancebot uses the command.

Did you:

  • [x] Join the Python Discord Community?
  • [x] If dependencies have been added or updated, run pipenv lock?
  • [x] Lint your code (pipenv run lint)?
  • [x] Set the PR to **allow edits from cont...
odd spireBOT
regal archBOT
#
jb3
[python-discord/bot] New branch created: mods\-role\-in\-allowed\-mentions
#
[python-discord/bot] branch deleted: mods\-role\-in\-allowed\-mentions
odd spireBOT
oak estuaryBOT
odd spireBOT
regal archBOT
odd spireBOT
regal archBOT
#

It's been a concern lately of tags cluttering the bot's command namespace. To mitigate the issue, we can split certain tags into groups, and then having a lot of related tags under that group won't be an issue.

For example, !intents can become dpy intents, and then we can add however many tags we want under dpy.

The tag group's md files can be stored in the same folder under resources.

regal archBOT
odd spireBOT
regal archBOT
oak estuaryBOT
odd spireBOT
odd spireBOT
oak estuaryBOT
odd spireBOT
odd spireBOT
regal archBOT
odd spireBOT
regal archBOT
regal archBOT
odd spireBOT
regal archBOT
odd spireBOT
regal archBOT
odd spireBOT
regal archBOT
odd spireBOT
regal archBOT
odd spireBOT
regal archBOT
odd spireBOT
regal archBOT
#
jb3
[python-discord/site] New branch created: joe/pipfile\-relock
odd spireBOT
regal archBOT
odd spireBOT
regal archBOT
#

I want to take a step back for a moment, because I feel like my original description didn't convey my full thoughts properly.

This is how I envision things working:

Overview

We have the Filtering (or however we name it) cog. The cog has an on_message listener, and for each message it asks the question: what should I do with it?

What should the cog do with the message?

To answer that question, it has an array of filter lists at its service, each represented as an instance o...

regal archBOT
regal archBOT
odd spireBOT
regal archBOT
odd spireBOT
regal archBOT
#

Thanks for the PR! I have some minor recommendations to make this command a bit more robust to user input. Otherwise the command is fun and simple, so the rest of my review will focus on the overall contribution process and things to keep in mind for the future.

Commit messages!
Commit messages are hugely important in the review process and also looking back if we're curious why a command was coded a specific way. I'd recommend adding reasoning as to why you're add/removing/changing ...

#

You removed the trex from the documentation but I'm still able to call it technically. The trex is also the only one where .cowsay trex errors out with 400 Bad Request error. It's probably worth catching it or forcing trex to be passed as an invalid character and call the error embed.

It would help in general if in the commit body of your commit messages you include some reasoning as to why things are removed/added/the way that they are.

#

Even though you check if the text is less than 150, for some of the bigger ASCII art characters, like the dragon, I can still generate something too large for discord to process. It results in a Bad Request error that is currently not getting caught or dealt with. It would be worth it to add a check to make sure than the final ascii art you paste is under the discord length or completely remove the ability to use some of the larger ascii characters.

odd spireBOT
regal archBOT
#

I don't think we should keep text a default value in the parameter as when one specifies the character to be a Dragon, the text which should come is I'm a Dragon, but here it would be I'm a Cow.

One possible solution is to keep the text None, and then check using a if statement, and if it is None, we can put in the default string

if not text:
  text = f"I'm a {character}"
#

Another comment, rather than putting these characters manually, we would have to change the documentation whenever a new character is added. This wouldn't something nice.

A possible solution to this is to use help= parameter in discord.ext.commands and pass in the characters to it, using f-strings. The characters can be seen like this:

cowsay.char_names
night lilyBOT
#
Sir Lancebot

Connected!

odd spireBOT
regal archBOT
regal archBOT
regal archBOT
#

Something that bothers me is that it isn't clear which embed relates to which message link, which is exacerbated if any of the messages error out and get skipped. Add the message link in the title (but then the embed is even bulkier)? in the footer? just the message ID?

Yeah, I would add the message ID in the footer, I had tried adding the message link as the Title and Footer but then it breaks (half link in line, second half in the next line) due to embed lengths and the embed is bulkier.

regal archBOT
odd spireBOT
odd spireBOT
regal archBOT
odd spireBOT
regal archBOT
regal archBOT
odd spireBOT
odd spireBOT
odd spireBOT
regal archBOT
regal archBOT
#

Hello,
I have already made both the commands (rock-paper-scissors and coin-flip) long ago. I have also shared the code in #dev-log but I didn't receive a definite reply which was why I accidentally forgot about it. I'm ready with the code now, I just need some help getting it added to Sir Lancebot.
Maybe we can discuss further on PyDis, I'll hit you up.

odd spireBOT
oak estuaryBOT
odd spireBOT
regal archBOT
#

You can get None from get_channel if the cache isn't ready yet. Usually this problem is circumvented by awaiting the wait_until_guild_available event on the bot instance. Although I guess it's not much of a concern here since it's only called from a command.

From the docs, it looks like if you do get None here it will kick the user from voice, and L85 will fail since you cannot reconnect them.

regal archBOT
#
[python-discord/quackstack] New branch created: no\-dev\-deps\-in\-dockerfile
#
[python-discord/quackstack] branch deleted: no\-dev\-deps\-in\-dockerfile
regal archBOT
odd spireBOT
regal archBOT
regal archBOT
#

Thank for the PR; this is looking fantastic! I only have a few minor comments (only about the file names and structure).

Many files are named things like Python_Earth_Day-33.png. Something like earth_day.png or icon.png would seem more logical imo.

The static folder includes Python_Earth_Day-33.png, which is a duplicate of the server icon that exists in /server_icons. I think this can simply be removed.

The svg for the banner seems to have the strange additional space on around...

#

Thank you!

The space is not intended.... Gustav said he will check on his end and show
me how to fix it in case it is an illustrator export thing.

On Wed, Apr 21, 2021 at 12:25 PM DawnOfMidnight @.***>
wrote:

@.**** approved this pull request.

These are amazing! Wonderful job!

Also, there are borders around the svg banner. Was that intended?

โ€”
You are receiving this because you authored the thread.
Reply to this email directly, view it on GitHub
<https://gi...

regal archBOT
#

which simply calls the execute of each of its entries

It is not so simple because the order of execution could be important (e.g. it can't DM a user that's banned).

The class also defines what action should be taken, using a method, let's call it execute.

Another type of entry is one which decides whether any action should be taken at all (for example if the message was sent by a staff member). For this entries will have another method, a predicate, which will return a boolea...

#

When a mods turns off mod pings, a confirmation is sent to inform the user that their pings have successfully been turned off.

In this confirmation, we currently include the time at which it is due to be sent, this time is in UTC.

I propose we refactor this part of the code to instead use an Embed, with a the timestamp field.
https://github.com/python-discord/bot/blob/ce819ade482e82ecbc474bce5fb8ac9dd8b37b40/bot/exts/moderation/modpings.py#L107
This would mean that the time would auto...

#

These changes don't quite seem to work on my end. For example, static/banner.svg includes the entire document you used when designing this. Properly adding this banner or logo as an svg seems close to impossible using Illustrator, so I'd be fine with either not including the svgs, or that you include the SVGs, without the shadow. Both ways are fine with me. I'd rather have this merged soon (since the event starts any time now) and fix the shadow later, if we can.

The earth seems to have ...

odd spireBOT
regal archBOT
#

The doc commands !d if and !d for (and possibly others) are grabbing from django docs. The expected behavior is the python syntax for if/elif/else.
!d label.if kind of pulls the right doc, but it's the grammatical usage and not very helpful for trying to understand the usage.
Maybe there could be some dedicated tags for them?
I've written a suggestion explanation for a dedicated !if-else tag here. This is just how I might explain it. Maybe it should be simplified, but I'll leave ...

regal archBOT
#
[python-discord/sir-lancebot] New branch created: update\-pr\-template
#

This is a rewording of the comments in the PR template, to hopefully clarify intentions.
It also removes unneeded fields.

Relevant Issues

Closes #685.

Description

Rewords portions of the PR template to clarify intentions through comments, and removing unneeded fields.

Make sure to view the raw file to see the comments.

Reasoning

This field is gone, shoo.

Screenshots

This field is gone as well.

Additional Details

This one too.

Did ...

odd spireBOT
regal archBOT
odd spireBOT
regal archBOT
odd spireBOT
regal archBOT
regal archBOT
regal archBOT
regal archBOT
#

Relevant Issues

Closes #33

Description

This PR does few miscellaneous changes and the major change: allows to pass options while generating a cute man duck.

Miscellaneous Changes:

  • Moves all Color named-tuples to src/colors.py, to keep them together and not define them at multiple places.
  • Changes ManDucky named-tuple to only hold image as rest options aren't needed.

Major Change

This PR adds two models, one for the Dress Colors of man-ducks and oth...

regal archBOT
regal archBOT
regal archBOT
#

It is not so simple because the order of execution could be important (e.g. it can't DM a user that's banned).

In the schema shown in the site issue, there is a type of setting in a separate table called filter_action, which bundles the user-facing actions (it might be better to move the message deletion to a normal setting), so it will handle both DMing the user and issuing the infraction.

These could be combined into a single function that takes some sort of FilterContext object...

regal archBOT
regal archBOT
#

Two things to bear in mind here:

  1. The objects derived from pydantic's BaseModel support dot access, so it would be better in all cases where you're subscripting the objects like this to just do it like options.colors.
  2. Objects derived from pydantic's BaseModel are not mappings, and as such can't be used with **, instead to use them like this you can call options.colors.dict() on them.

Do note that this comment also applies to the other places in the file you do this on the next...

regal archBOT
#
[python-discord/sir-lancebot] New branch created: fix\-reddit\-index\-error
#

Relevant Issues

Closes #701

Description

Changed a list slice from [1] to [0].
Added a min check for length of the respond, before shuffling.

Reasoning

If a subreddit has <2 posts, the posts[1] check would fail with an IndexError.
If the subreddit had less than 5 posts, then the k=5 check would also error. These changes harden the command for these edge cases.

Did you:

odd spireBOT
regal archBOT
odd spireBOT
regal archBOT
odd spireBOT
regal archBOT
regal archBOT
#

Description

Commands that echo user input have the vulnerability of allowing embedded images where they otherwise should not be allowed. It manifested today multiple times through the catify command, but has implications for other commands such as uwu and randomcase.

Steps to Reproduce

  1. Use the catify command with a CDN link.
  2. The bot should embed the link (even if the user can't).

Expected Behaviour

  1. The link is escaped.

Actual Behaviour

The ...

#
[python-discord/sir-lancebot] New branch created: link\-suppressing
#

Relevant Issues

Closes #703

Description

I've added a helper that does a regex search through a given string, looks for http or https, and surrounds that with <>. I've called it in places I could find where the bot tries to echo part of, or the full user input back to the user.

Reasoning

Regex seemed like the most simple way to deal with finding links. This solution does not take into account that an already escaped link will end up with weird extra brackets...

odd spireBOT
regal archBOT
#

Description

Unloaded cogs should be saved in redis, and should not be loaded after restarts.

Reasoning

From time to time, we may end up with problematic code that we choose to unload. This can happen for many reasons, from slight oversights to huge breaking bugs. The main point is, when we unload something, we usually don't want it loaded again until the problem is addressed.

Unloading right now works well enough, but it does not persist across bot restarts. There is no go...

odd spireBOT
regal archBOT
regal archBOT
#
[python-discord/sir-lancebot] branch deleted: link\-suppressing
night lilyBOT
#
Sir Lancebot

Connected!

odd spireBOT
regal archBOT
#

I don't agree that we should have it when most of the time most people won't use it. Screenshots may be useful when deciding what looks best, but I still expect reviewers to actually test out the features they are approving (because when they don't, we end up with broken code. ask me how I know).

There is also something to be said about keeping the template concise and limited to what is absolutely required, something I hope to address in the other templates once we're done here.

#

Yup, that's mostly what I had in mind. To answer a few of your points:

  • Trying to figure out where a module might have been moved is not something I think is worth much time or effort.
  • Any cog that should've been unloaded, but was not found will just be ignored.
  • Once the bot reconnects, it'll send out a ping in some channel (preferably hidden), to alert core-devs, or anyone with permissions to unload cogs, much like defcon (used to?) does. Logging an error will have the same effect, b...
regal archBOT
#

The reason for adding it in the first place was to address concerns about who is approving issues. Currently, anyone with write permissions (all staff members) can add the approved label. The conversation was linked in the issue, but [here](#dev-core message) it is again (staff channel). I don't mind it either way, but the consensus at the time was approving issues is a core dev responsibility, we should outline that.

regal archBOT
odd spireBOT
regal archBOT
regal archBOT
#

Okay! This now works perfectly when giving it correct options, however if I give it an invalid option, for example i set the outfit to aaaaaaaaaaa rather than a valid one, instead of getting a 400 bad request I get a 500, so I'd like to see that handled internally too, here's how it's done in normal duck generation, but you may want to refine it: (line 68, ducky.py)

        if equipment and equipment not in cls.equipments:
            raise HTTPException(400, "Invalid equipment pro...
odd spireBOT
regal archBOT