#dev-contrib

1 messages Β· Page 151 of 1

static canyon
#

This is what _check_filter corrently does

unknown.png
cold island
#

yeah ik

static canyon
#

I'm just thinking it makes sense to add it there

#

Since the whole purpose of that function is to check whether we want to run the filters or not

#

Which is exactly what the code I'm adding does

cold island
#

It's a helper function which takes the common actions that need to be made to check whether a filter should be run on a particular message. The change you're suggesting changes the purpose of the function, a purpose which doesn't fit any of the other places which use it aside from that particular event.

#

That function is also invoked from _filter_message, so you'd need to add the before there as well

#

I don't think it's necessary to propagate the before through several functions just for this

static canyon
#

Then what about moving _check_filter?

cold island
static canyon
cold island
#

The code you're addresses two states of a messages, instead of taking one message and checking it

#

This seems like a whole load of refactoring just for this

static canyon
cold island
#

You're making the function deal with something that most other places don't need. I don't think that improves the code

static canyon
static canyon
cold island
#

Any other place which needs to run this check

static canyon
#

The only other call is in filter_eval

cold island
#

And on_message

static canyon
#

No?

#

That's _filter_message not _check_filter

cold island
#

Which runs check_filter

static canyon
#

The only place that runs _check_filter is _filter_message (where _filter_message is called from on_message_edit and on_message) and filter_eval (where filter_eval is called after !e)

cold island
#

Look, I'm not really hellbent on stopping you, but this seems a little excessive to me. That function is not even going to exist in the new system

static canyon
#

I mean given that it's used elsewhere and would break those cases I guess it shouldn't be done

#

On its own I think it's fine though

cold island
#

On its own yes

#

But that function is used in multiple places which don't fit this use case

static canyon
#

Yeah

#

I'll leave it as-is

#

So I put the code I'm adding before the _filter_message call in on_message_edit?

cold island
#

That's what I'd do

#

Could also do it right at the start

#

Since if you're exiting early you don't need the delta

static canyon
#

Yeah, true

cursive relic
#

Is the .wiki issue yet merged?

static canyon
# cold island Could also do it right at the start

So```py
@Cog.listener()
async def on_message_edit(self, before: Message, after: Message) -> None:
"""
Invoke message filter for message edits.

    If there have been multiple edits, calculate the time delta from the previous edit.
    """
    # We only care about changes to the message contents/attachments
    if before.content == after.content and before.attachments == after.attachments:
        return

    if not before.edited_at:
        delta = relativedelta(after.edited_at, before.created_at).microseconds
    else:
        delta = relativedelta(after.edited_at, before.edited_at).microseconds
    await self._filter_message(after, delta)```?
#

Should I update the docstring or is the comment enough?

cold island
#

Something like that yeah.. would need testing ofc

cold island
static canyon
#

I'll leave the docstring as-is then πŸ‘

#

Now to test

clever wraith
#

You could add a comment saying you want to do this because you don't want to re-run on pin and embed removal

cold island
#

That's a good idea yeah

static canyon
# 
        # We only care about changes to the message contents/attachments, not pin status or embeds etc.```something like that?
clever wraith
#

seems good enough

static canyon
#

πŸ‘

cursive relic
#

Somehow I get all notifications from github either 2-18 hours late

static canyon
#

@cold island how do I go about testing when the test server doesn't have mod-logs?

static canyon
dusky shoreBOT
clever wraith
static canyon
#

Except they aren't going there lol

#

Ah wait I'm dumb they are sort of

cursive relic
static canyon
#

It's because I've not blacklisted stuff I think

clever wraith
#

oh yeah, the default config has a few chat blacklisted

#

I remember it took him like 15mins to figure it out, heh

static canyon
#

I've blacklisted bit.ly but it's not triggering a log

#

The only thing that does is sending a discord invite but I can't use that for testing since it deletes the message

#

And the whole point is that I need to be able to edit to see if it re-sends the log

#

Ah, figured out a way

#

And of course the code doesn't work lol

#

It is working, it's someone else's bot sending the log 🀦

cold island
#

Is it mine? I bet it's mine

cursive relic
#

what is the branch name for sir-lancebot#933 I don't seem to find it for some reason

dusky shoreBOT
cold island
#

You can unload the ext, the prefix is ~ @static canyon

cursive relic
clever wraith
#

wikipedia/933/lock-paginator-to-author

#

You can see it just under the PR title

#

Nipa-Code wants to merge 1 commit into python-discord:main from Nipa-Code:wikipedia/933/lock-paginator-to-author

cursive relic
#

oh...

static canyon
cursive relic
#

what was the command to set branch x to upstream or what ever it's called to push changes there

clever wraith
#

git push -u origin $branchname

#

so for example git push -u origin wikipedia/933/lock-paginator-to-author

#

if you ran it with -u once you can go ahead and just type git push

cursive relic
#

thx

cursive relic
#

and if I do git add before that command, where will the changes be pushed? Still to the one specified?

clever wraith
#

you need to code, add, commit (potentially repeat from the start), then push

cursive relic
#

I see

#

uhm

#

it seems to have made pushes to wrong branch

#

it didn't update the pr, instead it made this

unknown.png
#

is it now pushed to bot or something?

clever wraith
#

yes, it should be live now

cursive relic
#

now I have some how successfully made 2 prs to the bot

clever wraith
#

Congrats!

austere hornet
#

Congrats!

static canyon
#

@cold island have created bot#1937; would appreciate review when you're free πŸ‘

bot#1926 also needs a re-review (have addressed your prior review)

dusky shoreBOT
short snow
cold island
stable mountainBOT
#

bot/exts/backend/error_handler.py line 245

elif isinstance(e, errors.ArgumentParsingError):```
cold island
#

We don't raise it anywhere

#

So I just tried to do that

#

And it didn't reach the error handler

#

Oh wait

#

Nothing like rubber duck debugging

molten perch
#

Hey! Is there a consensus on whether we should use a test DB or a mocked DB for API tests? I'm a bit lost as to which I should implement and the project in the future. πŸ˜„

remote wigeon
#

Both have their advantages and disadvantages

#

A proper db will always give more accurate results, at the expense of a more complicated setup

gritty wind
#

This is the part that I'm still not convinced about. Why would the DB return more accurate results

molten perch
#

Maybe, the relationships and similar database features. (Though, the should be working as expected. )
(I'm literally working against myself πŸ˜‚)

gritty wind
#

Again, that's not something that we should be testing with our unit tests for routes...

#

If we want to, we can set up a separate integration test suite

#

But, anyways, I'm not sure what you even mean fully tbh

#

If there's a problem with the modeling, won't that be reflected in the pydantic models when you go to use them

molten perch
#

I don't think so, no.
If something's wrong with the relationships (although, in theory, and practice it should be fine) and you perform like a DELETE operation and something goes wrong, pydantic will be of no use.
However, I accept your view, I was just curious if you have decided which method to implement in the end.

gritty wind
#

Don't take my view as fact, let's wait for volcy

molten perch
#

Sure, I'm gonna implement the method you decide on. πŸ˜„

#

Also, the rest of the PR should be fine. (regarding the endpoints overall)

clever wraith
#

It is minor so I don't think there's any point in PRing it, but @cold island should existing filtering bugs/oversights be discussed in new issues (bot#1938) or in the filter rework pr?

dusky shoreBOT
cold island
#

Anything that isn't a major design change is fine

clever wraith
#

Alright, thanks

cold island
#

I refactored everything at least three times

green oriole
#

I'm not sure how relevant it is to make unfurling happen on the CF edge (CC @gritty wind), I don't see the added value other than not exposing our IP to bit.ly and stuff

gritty wind
#

It just makes it easier to have it be a blackbox we throw URLs into it with minimal concern for safety and resources

#

Partly why I was asking about execution limits on workers

#

Why put a depth limit when we can just have CF hammer it

#

But either would work

green oriole
#

I guess we do have CF unlimited, so it isn't that bad

molten perch
#

Hey! Can I get a few reviews on site#572? It should be fine as isℒ️ πŸ˜„

dusky shoreBOT
last patio
#

on it

#

oh i already did

#

if we really wanted to go-nuts we could probably do that delete in a single statement

#

but hell do i know how that would look in django orm lol

#

besides, this is more understandable for most people

molten perch
#

Yeah, it's pretty straightforward that way. πŸ˜„
(btw. thank you for reviving the PR, I totally forgot about it πŸ˜„ )

last patio
#

hahaha no problem

stable mountainBOT
#
It has arrived!

Here's your reminder: life got busy
[Jump back to when you created the reminder](#dev-contrib message)

dim pelican
stable mountainBOT
#
You're the boss!

Your reminder will arrive on <t:1636199653:F>!

ID: 3435
austere hornet
#

Lol

static canyon
#

@austere hornet You really shouldn't be committing code with linting errors

#

Have you set up precommit?

#

That would stop this from happening

#

precommit honestly helps make the commits much nicer

#

It seems 7/32 of your commits are linting, and 9/32 are just merging main (so arguably half of the commits weren't needed / could be avoided). You only need to merge main at the end of the project, rather than as-you-go because otherwise it just clutters the commit history which is a bad thing to do when it can be avoided

#

@clever wraith for bot#1937, would a check for len(before.embeds) < len(after.embeds) # embed(s) added suffice? I don't want to be rerunning when an embed is removed since there's no way it will make the message now trigger (it would've already done so)

dusky shoreBOT
clever wraith
static canyon
#

checking if the after state is a subset of the before case would cover all cases
So do you meanpy all(( embed in before.embeds for embed in after.embeds ))?

#

That seems wrong actually

#

The before and after would need to be the other way round

#

"If everything in before is in after"

clever wraith
#

what you've written seems fine

static canyon
#

That won't work for the case I gave though

#

Because if after has more items than before then it's impossible for everything in after to be in before

#

So they'd have to be swapped, no?

clever wraith
#

i was referring to this

if not all((embed in before.embeds for embed in after.embeds)):
    run_filter
#

what does all do for an empty iterable?

static canyon
clever wraith
#

!e print(all(()))

stable mountainBOT
#

@clever wraith :white_check_mark: Your eval job has completed with return code 0.

True
static canyon
#

!e py print( all(( number in [1,2,3] for number in [] )) )

stable mountainBOT
#

@static canyon :white_check_mark: Your eval job has completed with return code 0.

True
static canyon
#

Yeah, that's an issue I guess

clever wraith
#

well if we have no embeds in the after state then the embed was removed

#

len(embeds) will at most be 1

static canyon
#

I don't know what you mean by that

#

right

#

Seems a bit of a niche but fair enough

clever wraith
#

its quite niche, yeah, like I said I don't expect us to encounter this

green oriole
#

I mean, they can just not send a rich embed

#

And use whatever type the link previews are

static canyon
# 
        # We only care about changes to the message contents/attachments and embed additions, not pin status etc.
        if all((
            before.content == after.content,  # content hasn't changed
            before.attachments == after.attachments,  # attachments haven't changed
            all(embed in before.embeds for embed in after.embeds)  # new embeds haven't been added
        )):
            return
```So like this? @clever wraith
green oriole
#

It sounds pointless

#

The only filter we run right now is rich embed

#

If we did filter embed texts maybe it would be worth doing

static canyon
green oriole
#

I mean, I really doubt a rich embed will appear after an edit

#

And even if it does we would get a ping for a user seeing it

static canyon
#

Based on that logic you could just completely remove the filter system and rely on other users reporting if/when they see

green oriole
#

I think we have to take into account the probability of that happening

#

And here it is so slim that I don't think we should care

static canyon
#

I'm making changes to the file anyway, I might as well account for all cases whilst I'm adjusting things 🀷

green oriole
#

Embed equality is also pretty jank

static canyon
#

I agree it seems unlikely, which is why I originally didn't bother, but if I'm improving the logic anyway it should include all improvements, however minor

clever wraith
#

it's less of an improvement and more of a "retaining a feature we already have", since in its current state we'd get a ping for that case. its just one additional line, so i think its fine to add

anyway, i dont have a strong opinion but @static canyon that looks good to me

green oriole
#

If you can make sure it works, I am fine with it

#

But it will probably be a little bit of a pain to test

clever wraith
#

time to selfbot lemon_fingerguns_shades

static canyon
#

time to setup a selfbot

#

I can't be bothered to test so I guess I'll just leave it as-is

#

Should I do my len(embeds) approach so that it still semi-accounts for embeds @clever wraith

green oriole
#

Honestly, why do we even re-run messages that already pinged in the last, say, 30 secs

clever wraith
green oriole
#

The len thing will retrigger if the embeds are removed

static canyon
#

No it won't

#

Well

#

Hmm

# 
if len(before.embeds) >= len(after.embeds):
    return```
clever wraith
# 
        if all((
            before.content == after.content,  # content hasn't changed
            before.attachments == after.attachments,  # attachments haven't changed
            len(before.embeds) < len(after.embeds)    # new embeds haven't been added
        )):
``` this'd work, no?
static canyon
#

That won't trigger for embeds being removed

static canyon
#

Wait

#

Yeah

#

You're saying a new embed has been added

#

Since there's more embeds after than before

static canyon
#

Ah wait no that wouldn't go there

#

Since you don't know if it pinged or not

#

So I guess the reason we don't is we don't track the messages that pinged and therefore can't

green oriole
#

Do we not?

#

Well, I guess we could be tracking that in a short deque

#

Like a tuple message ID, timestamp

static canyon
#

Doesn't look like it

# 
                        # If the message is classed as offensive, we store it in the site db and
                        # it will be deleted it after one week.
                        if _filter["schedule_deletion"] and not is_private:
                            delete_date = (msg.created_at + OFFENSIVE_MSG_DELETE_TIME).isoformat()
                            data = {
                                'id': msg.id,
                                'channel_id': msg.channel.id,
                                'delete_date': delete_date
                            }

                            try:
                                await self.bot.api_client.post('bot/offensive-messages', json=data)```
#

That's the closest thing

#

Andpy self.bot.stats.incr(f"filters.{name}")

clever wraith
#

I'd rather the filters ran again but without a mod ping

static canyon
#

Eh, editing I guess

clever wraith
#

yeah, that

static canyon
#

So I've now got```py
@Cog.listener()
async def on_message_edit(self, before: Message, after: Message) -> None:
"""
Invoke message filter for message edits.

    Also calculates the time delta from the previous edit or when message was sent if there's no prior edits.
    """
    # We only care about changes to the message contents/attachments and embed additions, not pin status etc.
    if all((
        before.content == after.content,  # content hasn't changed
        before.attachments == after.attachments,  # attachments haven't changed
        len(before.embeds) >= len(after.embeds)  # embeds haven't been added
    )):
        return

    if not before.edited_at:
        delta = relativedelta(after.edited_at, before.created_at).microseconds
    else:
        delta = relativedelta(after.edited_at, before.edited_at).microseconds
    await self._filter_message(after, delta)```
#

Does that look good? @clever wraith @green oriole

green oriole
#

What do you do with delta?

static canyon
#

It's used in _filter_message in the embed detection

#

If it's < 0.1 secs or something then it ignores since it's a double-fire

# 
                    if filter_name == "watch_rich_embeds":
                        # If the edit delta is less than 0.001 seconds, then we're probably dealing
                        # with a double filter trigger.
                        if delta is not None and delta < 100:
                            continue```
#

That's the only place I think

#

CC @green oriole

green oriole
#

Hmm alright

static canyon
#

So should I commit this?

clever wraith
#

lgtm

#

boolean logic sucks tho, took me a while to get why it wasn't len(before.embeds) < len(after.embeds)

static canyon
#

lol

#

I've done loads of it at school etc. so just used to it now

static canyon
dusky shoreBOT
brisk brook
brisk brook
static canyon
#

It's not a big deal, just makes lives a bit easier

#

Although I suppose in this PR we'll have to squash anyway

static canyon
#

lol

stable mountainBOT
rapid igloo
dim pelican
trail pilot
austere hornet
austere hornet
austere hornet
#

Need to read better smh

trail pilot
short snow
#

@brisk brook zig left a few comments on your review on the Enhanced incidents PR, could you look and comment on them?

brisk brook
#

Sure hold on

short snow
#

Cool thanks

short snow
#

Is this fine for thread channels? @brisk brook

Screenshot_from_2021-11-07_01-31-19.png
brisk brook
#

Maybe check how the styling looks for the embeds that get sent when someone opens a thread?

short snow
#

this, (modlog has no support for thread message edits/delets/etc yet)

Screenshot_from_2021-11-07_01-33-17.png
#

Also there is no handling for no message content now, so should i just show this i.e. [No Message Content]

Screenshot_from_2021-11-07_01-34-48.png
brisk brook
brisk brook
#

Bot Commands/#bot-commands - 'test'?

short snow
#

Yeah that works πŸ‘

cold island
#

Better be explicit

short snow
#

;-; choose one lol - even I like [No Message COntent] personally

vocal prairie
#

I like the [No Message Content] just reading it

short snow
#

@brisk brook pushed πŸŽ‰

molten perch
#

Hey! Could someone take a look at api#25 or site#572 ? I'd appreciate it. lemon_fingerguns_shades

dusky shoreBOT
dusky shoreBOT
molten perch
#

~~I wrote "site #572" first then edited :/ ~~

austere hornet
#

Yeah was gonna ask the same thing

fallen patrol
#

@gritty wind can I submit it a pr to channel unlock the github command?

gritty wind
#

I have no idea what the permissions are meant to be

fallen patrol
gritty wind
#

I thought we removed that command entierly

#

Or at least had an issue open to do it

fallen patrol
#

No, that was the issue and pr commands of it which were removed

timid sentinel
#

I think there's a PR that removes it

fallen patrol
#

The whole command was not removed

#

But two parts of it were

#

.issue

#

That is the command that is being removed IIRC

#

.help issue

static canyon
timid sentinel
#

Ah yeah

fallen patrol
#

Sir-lancebot#778

dusky shoreBOT
static canyon
fallen patrol
#

Lmao @timid sentinel you're one of the reviews that still needs to pass on that pr

#

Say

#

Meta#93

#

Nice it just fails on discussions

timid sentinel
fallen patrol
#

O, I thought you gave a requested changes review ablobsweats

trail pilot
#

@rapid igloo Committed changes for sir-lancebot#909

dusky shoreBOT
short snow
obtuse arrow
#

~~Anybody else have every bot test fail due to permission errors when running the poetry task in WSL? ~~
EDIT: Solved it, for some reason the bot log was owned by root instead of my user and the tests couldn't write to it.

rapid igloo
# trail pilot <@!544825455509897227> Committed changes for sir-lancebot#909

hey, looks like the lint task failed: bot/exts/utilities/challenges.py:267:1: D202 No blank lines allowed after function docstring. Just need to remove the blank line and should be good to go
(perhaps you should squash it with the last commit if you can, since it's a little unclean to add something in a commit only to remove it in the next πŸ˜„)

trail pilot
#

Committed

molten perch
#

Since site#618 is merged, can I open a corresponding PR on the API repo, so that we won't forget adding it?

dusky shoreBOT
thorny obsidian
#

@brisk brook git mv doesn't do anything different from renaming and moving files.

vocal wolf
brisk brook
#

Hold on let me do a test

thorny obsidian
#

So do you know how git blame attempts to follow renames and files moving?

brisk brook
# 
On branch main
Changes to be committed:
  (use "git restore --staged <file>..." to unstage)
        renamed:    README.md -> README.rst

Changes not staged for commit:
  (use "git add <file>..." to update what will be committed)
  (use "git restore <file>..." to discard changes in working directory)
        modified:   README.rst
brisk brook
# thorny obsidian So do you know how `git blame` attempts to follow renames and files moving? 
PS C:\Users\%username%\Projects\git-mv-test> git blame README.rst
408e35e0 README.rst (Bluenix 2021-11-07 19:51:36 +0100 1) Git mv test
408e35e0 README.rst (Bluenix 2021-11-07 19:51:36 +0100 2) ===========
^0fc5ba2 README.md  (Bluenix 2021-11-07 19:49:34 +0100 3)
^0fc5ba2 README.md  (Bluenix 2021-11-07 19:49:34 +0100 4) This is a simple repository testing how Git's `git mv` command works.
^0fc5ba2 README.md  (Bluenix 2021-11-07 19:49:34 +0100 5)
^0fc5ba2 README.md  (Bluenix 2021-11-07 19:49:34 +0100 6) The questions is whether it helps `git blame`, which this repository attempts
^0fc5ba2 README.md  (Bluenix 2021-11-07 19:49:34 +0100 7) to demonstrate.
#

See?

green oriole
#

If you don't use git mv, git blame will just try to guess whenever it is a move or not

#

If you do use git mv then it knows for sure you moved it

brisk brook
#

But yeah they're all separate cogs that have been merged, so it might be a little harder than what I have done in my test

green oriole
#

The best way to have the history follow is to just rename in a single commit, then move stuff around

brisk brook
#

https://devblogs.microsoft.com/oldnewthing/20190514-00/?p=102493

The Old New Thing
Raymond Chen
Mundane git tricks: Combining two files into one while preserving l...

Suppose you have two files that you want to combine into one. Let’s set up a scratch repo to demonstrate. I’ve omitted the command prompts so you can copy-paste this into your shell of choice and play along at home. (The timestamps and commit hashes will naturally be different.) git init >fruits echo apple git add fruits git commit --author="Al...

#

Yeah this blog post explains how to rename two files into one

#

If we can't really get this to work I'd say we close the PR. I don't think it truly is worth it

thorny obsidian
#

From what I've read in the wikis and stuff, it boils down to:

Git has a rename command git mv, but that is just for convenience. The effect is indistinguishable from removing the file and adding another with different name and the same content.

It helps a bit because of how it does its staging, but ultimately trying to preserve git blame when doing a reorg is always a bit of a toss up.

git tries its best to track files by comparing the content in a file and seeing how similar it is. If you remove a file and add a file in the same commit it tends to do better, but it doesn't do anything special.

#

I don't think closing the PR because we can't perfectly preserve the git blame is a good move. At the end of the day that cog is a bit of a mess and I do want .timeleft to not be a top level command.

static canyon
vocal wolf
#

the other user doesn't seem to be active, hasn't responded to ping for a while

#

pretty sure I pinged them here as well

static canyon
#

🀷

grim herald
#

hmm

austere hornet
molten perch
#

Hey! Could someone take a look at api#27? It's literally a one-liner and a migration. (That is autogenerated, and still pretty straightforward)

dusky shoreBOT
fallen patrol
#

@gritty wind can i make a pr to add pip as an alias to pypi?

gritty wind
#

Sure

fallen patrol
#

thanks

fallen patrol
dusky shoreBOT
fallen patrol
#

the lag on that wow

#

...

#

interesting

static canyon
#

.ping

dusky shoreBOT
#
:ping_pong: Pong!

Gateway Latency: 115ms

fallen patrol
#

#bot-commands message

#

10 seconds.

fallen patrol
cold island
fallen patrol
#

^

#

you can see the effect with making a commit of just a rename, and then amending the commit with many edits to that file, git will no longer think it was a rename

thorny obsidian
fallen patrol
thorny obsidian
#

That isn't always possible unless you're doing a clean rename and move. Sometimes a file is going to get split up into multiple or moved and fundamentally changed. Especially if you're contending with multiple files that need to be somewhat combined and shifted.

cold island
#

Yeah it can get hairy. I might sometimes rename to the name of one of the new files and then move the code out of it

cold island
fallen patrol
#

@vale ibex there's a mistake in one of the commands in patsy, I have a commit ready to make a pr with
poetry run precommit -> poetry run task precommit

#

merged, neat

vale ibex
fallen patrol
#

already merged πŸ˜›

#

patsy#3

dusky shoreBOT
vale ibex
#

huh I don't see that in main

fallen patrol
fallen patrol
#

you did lol

vale ibex
#

ah well, I didn't expect anyone else to be working on this

#

let alone merging PRs lol

#

I'm gonna change that setting actually

fallen patrol
patent pivot
#

alertmanager is great i would use alertmanager as my health monitor dude i'm telling you all alertmanager is geat

green oriole
#

did you force pushed to main chris lmfao

vale ibex
#

I was fixing a crashbackoff lol

fallen patrol
#

smh smh mh smh

vale ibex
#

git wasn't letting me push so I just forced it

#

now I see why

fallen patrol
#

git pull/ git rebase

green oriole
#

lmfao

patent pivot
#

excellent

green oriole
#

what even is this pasta thing for again

#

No description, website, or topics provided.

patent pivot
#

lol

vale ibex
fallen patrol
#

!otn a message pasta

patent pivot
#

it's the help channel message thingy

green oriole
#

right

#

why is it about pastas though

#

I mean as PyDis' official Italian I approve, but why

#

it makes me hungry

vale ibex
#

have you read the readme?

fallen patrol
#

anyways, IIRC Co-authored-by: onerandomusername <genericusername414@gmail.com> @vale ibex, or i think you can pull down commit ce78440cf1c028b184339cf817a4f11d9feb4f30

vale ibex
#

it's literally the first sentence lol

#

and the purpose is the next section

green oriole
#

ooh, that's the name of the coconut guy

fallen patrol
#

chris should i make a new pr?

green oriole
#

but how did the cluster get a coconut?

#

there is no coconut in Frankfurt

vale ibex
fallen patrol
#

oh

#

before i do, do you want to add patsy to #dev-log ?

#

since we aren't getting anything except checks in #dev-log

green oriole
#

imagine having admin perms on patsy

patent pivot
#

lol

#

i will do that

fallen patrol
#

lmk when you have it, and i'll submit the pr πŸ™ƒ

patent pivot
#

lol

#

it's not that urgent

patent pivot
fallen patrol
#

lol it didn't go to dev-log

patent pivot
#

oh

#

oh lol i didn't change the events

fallen patrol
#

lol

patent pivot
#

wait so who just did

#

what

#

was that you @vale ibex

fallen patrol
#

wdym?

#

none of the pr events have gone to #dev-log

#

the only thing that has gone is the workflow that sends to that channel

patent pivot
#

yea but i opened the page again to add the events and someone did lol

fallen patrol
#

replica of GH-3 since Chris force pushed to main and lost the merge commit.

#

perfect description

vale ibex
patent pivot
#

ah

vale ibex
#

didn't have anything setup since I wasn't expecting others to contribute yet...

green oriole
#

probs chris

patent pivot
#

lol

rapid igloo
brisk brook
#

See the post I sent, I might try that out and open a PR that supersedes Tizzy's

rapid igloo
#

oh! it's just above, I didn't see lemon_sweat. gl, it'd be amazing if we could preserve histories from the merge files but the blame and changes for the preserved commits might look a bit weird for someone in the future if they didn't know about this, as they change different parts of a merged file.

gritty wind
fallen patrol
#

@gritty wind which pr is this referencing?

obtuse arrow
#

@patent pivot site#364 <- Is this still relevant?

dusky shoreBOT
static canyon
dusky shoreBOT
static canyon
gritty wind
#

I wouldn't worry about it

obtuse arrow
#

git blame is very useful, though.

#

I don't usually use it from the CLI, but there are IDE integrations that are super nice.

gritty wind
#

Yeah, github integration too

obtuse arrow
#

Pycharm has it by default and with VSCode I've been using GitLens.

gritty wind
#

But it isn't a blocker for this PR to lose a bit of history

obtuse arrow
#

Yeah, no big deal if it's just one file.

#

But I'd hesitate to wipe history for large numbers of files.

crude gyro
#

git blame is nice, but I think it's fine to lose blame for a single fine due to refactoring.

#

but I agree with dementati that it sucks to do stuff like.. run black over the entire codebase and ruin blame for like 80% of the files.

#

still, this isn't that.

green oriole
crude gyro
#

maybe. I've never seen anyone actually open a blackify a shitton of code PR that did that, though.

#

added that metadata, I mean

obtuse arrow
#

I feel like the best approach for improving styling is to just apply a "leave the campsite in a better condition than you found it" principle as part of working on other issues.

crude gyro
#

yeah. boy scout principle.

#

I agree

#

that's what my projects tend to follow

green oriole
#

Yep, that's probably the wisest choice

obtuse arrow
crude gyro
#

and The Pragmatic Programmer uses the term Kaizen to refer to little unrelated cleanups and improvements you make as you work on different parts of the codebase

#

which is a nice term

static canyon
crude gyro
#

I had a talk about kaizen at a show and tell at Dignio a few years ago, and now most of the engineers make these clearly labeled "kaizen commits" or "kaizen PRs" every now and then, it's nice.

green oriole
#

One of the main problem with the boy scout principle is automated linting

crude gyro
#

how do you mean

green oriole
#

You can't add a flake8-plugin or something similar to test for your new styling rule until the whole database uses this style

crude gyro
#

sure you can, just lint only new code

green oriole
#

hmm, but full codebase lints will fail, I guess you need a proper CI setup that will only lint the diff between your branch and main then

crude gyro
#

yes

green oriole
#

which sounds reasonable

crude gyro
#

it's pretty common

#

it's really the only non-disruptive way to introduce new linting tools into your toolchain in a large codebase

#

that old code has been running in production for years, your customers are happy, there's probably no real benefit to making huge, sweeping changes to it in some sort of mega-PR that adds new linting and cleans stuff up

#

engineers love to do that because it's satisfying to clean up clutter

#

but it's often risky

#

and the point of linting toolchains is to reduce risk

#

so usually you'll introduce CI that only looks at the diff, both for test coverage and code style.

#

the only thing I'd run the entire codebase through would be SAST

short snow
#

Isn't this valid for the API migration also? pithink

gritty wind
#

Probably not, what history are you trying to keep

clever wraith
crude gyro
#

white box testing, like bandit in Python.

#

finding security vulnerabilities

#

like.. passwords in plaintext in the code, or compromised dependencies

#

short for static application security testing

short snow
gritty wind
#

Most of the past changes from what I could tell are already formatted with black, so they do not apply to this

#

And if you're already at a stage where you can comfortably add it all in one go, then there is no reason to do it piecemeal as described

clever wraith
#

aah, bandit does some weird stuff sometimes

#

like STATIC_ROOT = "/tmp/static" was considered as an hardcoded password

crude gyro
#

yeah that's a bit weird. but no static analysis tool is perfect

#

better to get one more alert than you need than one too few

short snow
gritty wind
#

I was referring to the actual api, I don't see how site is related to that

#

Presumably we'd remove the api from site once the migration is done

#

Also, site doesn't follow black

clever wraith
#

plus we got to write a little "wtf?" in the codebase which is an accomplishement

short snow
#

Ah, yeah according to discussions we will be removing the API from the site once the migration is complete but after removing it from the site we will lose all the git history associated with it.

clever wraith
#

I think it is fine, the whole codebase is getting rewritten anyway, right?

brisk brook
short snow
#

Yeah it is, but things like why is the permissions model a BigIntegerField and not a simple integer field aren't answered, if we look at the git blame, it would be easier.

Note, I am not against having it or not having it, just saying it as the topic came up above

fallen patrol
clever wraith
#

doesn't it render in GitHub?

clever wraith
#

@gritty wind interesting way to supply urls thonking why don't you use a POST instead?

gritty wind
#

A get made more sense as the method:

The POST method submits an entity to the specified resource, often causing a change in state or side effects on the server.

#

The GET method requests a representation of the specified resource. Requests using GET should only retrieve data.

#

At the end of the day, it really shouldn't matter much

clever wraith
#

Well, either way I think url encoded urls is a much more reliable way to pass urls, heh

gritty wind
#

I really didn't give it as much thought as you did, because for the most part it doesn't matter

#

If you'd like to modify it to something else, feel free

clever wraith
#

hmm I can't really suggest changes since I use Ak's GH account

# 
function parseURL(url: string): string {
  const params = new URL(url).searchParams
  
  return params.get("url") || throw Error('InputError')
}```
#

I believe it should be as simple as that

brisk brook
#

Is throw an expression?!

clever wraith
#

Pretty sure that's a thing you can do, yeah

#

Okay maybe not

#

it isn't in the TS paste anyway

fervent sage
obtuse arrow
fervent sage
#

But for something like a field being a bigint rather than an int, they work pretty well

#

And given the general standards of review here I'm fairly sure the comment would get updated with the code, even if it was during pr review

brisk brook
#

The trick is to keep the comment as close to the code as possible

short snow
#

Yes comments do work for most of the cases, but git blame is still important - why is this here? / when was this introduced? / who should I contact if I have a question regarding this code? / etc

gritty wind
#

It’s generally a bad idea for single individuals to be responsible for remembering specifics like that

#

Because people forget, and people leave

#

Comments are forever 🀑

patent pivot
#

GitHub doesn't look at .git-blame-ignore-revs

#

https://github.community/t/support-ignore-revs-file-in-githubs-blame-view/3256

GitHub Support Community
Support ignore-revs-file in Github's blame view

In Git 2.23+, you can specify a list of commits to skip during git blame: https://git-scm.com/docs/git-blame#Documentation/git-blame.txtβ€”ignore-revs-fileltfilegt This is very useful for hiding non-essential commits such as bulk code formatting. A lot of projects are already using this format, e.g.: https://github.com/nodejs/node/blob/master/...

clever wraith
#

how sad

thorny obsidian
green oriole
#

That's how I've seen it be done

obtuse arrow
#

You can often infer useful information from commit messages and when a given commit was made, though. And even if you can't always rely on the original author being around, if they are, it tends to make figuring something out much much faster. I'm not saying comments aren't also useful, but git blame has just been so freaking useful in my career that I don't want people to discount it.

gritty wind
#

I use it frequently too, it's how we got the history for our CVE, but in this case I don't think it really matters, and in no case do I think it's worth blocking a project over

green oriole
#

It honestly wasn't hard to track the line

#

Even through a few moves

gritty wind
#

Javascript; the language where try/catch apparently doesn't try and catch shit

# 
  try {
    body = await request.json<any>();
  } catch (error) {
    return new Response(
      `Could not parse body, error: ${error.message}`,
      {"status": 200}
    );
  }
#

No idea what's up with this lol

#

Despite catching the error, and returning the response, it still outputs an error in console

green oriole
#

Who love JS'

molten perch
#

lol never seen <any> in JS in that context.

gritty wind
#

TS requires type annotations if you're explicitly converting to any

molten perch
#

Oh, so TS. πŸ˜„

vale ibex
# 
request.json<any>().then((body) => {
    console.log(body);
}).catch((error) => {
    console.error(error);
});
``` does this work?
gritty wind
#

Work to do what in this case

#

It'll still log an error to the console

vale ibex
#

ah yea,

gritty wind
#

I suppose I can try to resolve the response out of there

#

let me try that

vale ibex
#

helper util

green oriole
#

Oh right

gritty wind
#

Still not working, seems to be logging to console directly

#

I wonder if the json method does that

#

Regardless, the SO presents it as an alternative to try/catch, for performance reasons, not because one method doesn't work

#

Like my try/catch is working to return the correct response, but its still outputting an exception in console

#

Tbh I should probably just use the builtin JSON parser, it's what's done under the hood anyways

#

Yeah that's fixed it

molten perch
#

Hey! Sorry, for keep asking to review some of my PRs but api#25, could use some reviews. (Apart from the tests, of course, it's still dubious)

dusky shoreBOT
vale ibex
#

Anyone know what's happening here? async sqlalchemy ```
File "/usr/local/lib/python3.9/site-packages/sqlalchemy/dialects/postgresql/asyncpg.py", line 359, in _handle_exception
patsy-patsy-1 | self._adapt_connection._handle_exception(error)
patsy-patsy-1 | File "/usr/local/lib/python3.9/site-packages/sqlalchemy/dialects/postgresql/asyncpg.py", line 652, in _handle_exception
patsy-patsy-1 | raise translated_error from error
patsy-patsy-1 | sqlalchemy.exc.DBAPIError: (sqlalchemy.dialects.postgresql.asyncpg.Error) <class 'asyncpg.exceptions.DataError'>: invalid input for query argument $1: 126811506632294400 (value out of int32 range)
patsy-patsy-1 | [SQL: INSERT INTO users (user_id, opted_out) VALUES (%s, %s)]
patsy-patsy-1 | [parameters: (126811506632294400, False)]
patsy-patsy-1 | (Background on this error at: https://sqlalche.me/e/14/dbapi)

#

and yes, I'm using BitInteger

#

I've even checked in the psql DB, it's a BIGINT

#

I'd expect this error if using a normal integer type, but not sure where this is coming from

gritty wind
#

Could it be trying to do that validation outside of the DB for some reason?

#

Also why store ints when strings do the job 🀑

vale ibex
#

lol yea, thought I was being smart, since dealing with ints is easier

patent pivot
#

ints my friend

#

anything in pg logs?

vale ibex
#

nah it's not hitting the db

#

getting errored in asyncpg

gritty wind
#

But I mean, the error is accurate. It's way outside int32 limit

#

2,147,483,648 <<<< 126,811,506,632,294,400

vale ibex
#

lol yea, but BigInt is an int64

#

no idea why it's using an int32 here

gritty wind
#

That's why i'm wondering if it's using int 32 internally

#

Need some of that spicy source code plz

#

Not your source

#

asyncpg source

vale ibex
#

using asyncbinary so it's all stubs lol

#

lemme find a github

gritty wind
#

For some reason the sqlalchemy site isn't loading for me πŸ™ˆ

vale ibex
gritty wind
#

line 359 is something different

#

What version are you on

vale ibex
#

sqlalchemy >=1.4.17,<1.5.0 asyncpg 0.24.0

molten perch
#

Which model is this, if I may ask?
(I'm browsing the repo right now)

gritty wind
#

Seems version x.x.24 has that

vale ibex
# molten perch Which model is this, if I may ask? (I'm browsing the repo right now) 
from typing import Optional

from sqlmodel import BigInteger, Field, SQLModel


class UserBase(SQLModel):
    """A base model for storing information about users."""

    user_id: int
    opted_out: Optional[bool]


class UsersTable(UserBase, table=True):
    """A table for storing information about users."""

    __tablename__ = "users"

    user_id: int = Field(sa_column=BigInteger, primary_key=True)
    opted_out: bool = Field(default=False, nullable=False)
#

It's not API

#

it's for patsy

molten perch
#

I know. πŸ™‚

vale ibex
#

alr cool, it's all still local atm

gritty wind
#

This is raised by an execute?

vale ibex
# 
@router.post("/users/", response_model=UserRead)
async def create_user(*, session: AsyncSession = Depends(get_session), user: UserUpsert):
    """Creates a new user."""
    db_user = UsersTable.from_orm(user)
    session.add(db_user)
    await session.commit()
    await session.refresh(db_user)
    return db_user
#

both UserUpsert and UserRead are direct children of UserBase

molten perch
#

Could you try to sort of "bundle" BigInteger into a Column(from sqlmodel)?
Like: [...] = Field(sa_column=Column("user_id", BigInteger, primary_key=True)
I might not be right, but it's possible that it can't recognize it and just simply goes with int, or something? πŸ˜„

vale ibex
#

hah yea that's solved it

#

how strange

#

Thanks πŸ˜„

molten perch
#

No worries πŸ˜„

brisk brook
#

Wait did IDs get another digit?

patent pivot
#

maybe

#

no we're on 9XXXXX IDs now

#

then they'll get another digit

brisk brook
#

yeah nvm I was just interacting with those extremely old Discord users holy shit

#

127499098990444545 <- like wut

vale ibex
#

lol mines that low too

#

@mellow hare's is one less digit than that too

brisk brook
#

Yeah haha

mellow hare
#

LP's is the lowest I've seen so far

brisk brook
#

I am so used to interacting with super new accounts lol

mellow hare
#

But mine is pretty low, yeah

molten perch
#

Oh btw. @vale ibex the rest of the models might need to be patched as well. I'm not sure if the project is open for contributions, but I can take care of it if you haven't already.

vale ibex
#

Yea I've patched them all now πŸ˜„

#

Just going through and adding crud endpoints

#

i quite like async sqlalchemy

# 
@router.get("/users/", response_model=list[UserRead])
async def get_users(
    *,
    session: AsyncSession = Depends(get_session),
    offset: int = 0,
    limit: int = Query(default=100, lte=100),
):
    """Get a list of all users."""
    result = await session.execute(select(UsersTable).offset(offset).limit(limit))
    users = result.scalars().all()
    return users
#

very nice code

brisk brook
#

I am trying to test the tags PR by numerlor but the bot seems to be in a crash loop because of this: ```python
python-discord-bot-bot-1 | The above exception was the direct cause of the following exception:

python-discord-bot-bot-1 |

python-discord-bot-bot-1 | Traceback (most recent call last):

python-discord-bot-bot-1 | File "/usr/local/lib/python3.9/runpy.py", line 197, in _run_module_as_main

python-discord-bot-bot-1 | return _run_code(code, main_globals, None,

python-discord-bot-bot-1 | File "/usr/local/lib/python3.9/runpy.py", line 87, in _run_code

python-discord-bot-bot-1 | exec(code, run_globals)

python-discord-bot-bot-1 | File "/bot/bot/main.py", line 14, in <module>

python-discord-bot-bot-1 | bot.instance.load_extensions()

python-discord-bot-bot-1 | File "/bot/bot/bot.py", line 143, in load_extensions

python-discord-bot-bot-1 | self.load_extension(extension)

python-discord-bot-bot-1 | File "/usr/local/lib/python3.9/site-packages/discord/ext/commands/bot.py", line 732, in load_extension

python-discord-bot-bot-1 | self._load_from_module_spec(spec, name)

python-discord-bot-bot-1 | File "/usr/local/lib/python3.9/site-packages/discord/ext/commands/bot.py", line 663, in _load_from_module_spec

python-discord-bot-bot-1 | raise errors.ExtensionFailed(key, e) from e

python-discord-bot-bot-1 | discord.ext.commands.errors.ExtensionFailed: Extension 'bot.exts.info.information' raised an error: TypeError: init() takes 3 positional arguments but 4 were given

vale ibex
#

can you link the pr?

ebon heron
#

Could be used as a temp solution

vale ibex
#

Thanks, we're currently waiting for a bit until we decide to migrate/introduce a new lib to our bots

brisk brook
vale ibex
#

we're not in a rush for slash commands, so want to make sure that the one we do pick will have good maintainers

brisk brook
#

Maybe I should run docker-compose build to make sure the internal state is doing okay

clever wraith
#

inb4 said maintainers: python discord\ℒ️

vale ibex
#

maybe a docker-compose pull first

#

since this PR doesn't seem to edit the information cog

clever wraith
#

also check if you have untracked files

vale ibex
#

I came here for a reason

#

but I forget why now

#

oh well

clever wraith
#

relatable

brisk brook
ebon heron
clever wraith
#

oh I think I know, did you mean to help @clever wraith do her homeworks, @vale ibex ?

thorny obsidian
#

Do we have a strong use case yet for slash commands?

brisk brook
#

Not a blocker afaik

#

Other than improved UX from slash commands being in-UI

thorny obsidian
#

The only thing I can maybe think of is the new roles subscribing thing, but even that is slightly better suited to components

clever wraith
#

the only very nice use case I can see currently is the blacklist and whitelist commands

#

so you don't have to remember list names

static canyon
#

Or at least for me

vale ibex
thorny obsidian
#

how is that PR coming along?

clever wraith
vale ibex
#

waiting for review lol

clever wraith
#

domain_name maybe?

vale ibex
#

haven't had any comments yet

thorny obsidian
static canyon
clever wraith
#

ay

static canyon
#

filter_token, guild_invite, domain_name are the main three

vale ibex
#

bot#1868

dusky shoreBOT
cursive relic
#

Is it normal for channel to close in 12 minute in activity?

static canyon
clever wraith
#

filter_token is so weird, for me it would sound like Discord tokens or whatever

cursive relic
brisk brook
static canyon
vale ibex
#

If the claiment wasn't typing, then it'll close sooner

brisk brook
#

lol wasn't there gonna be a Pixels thing soon?

vale ibex
#

heh

brisk brook
#

We need to figure out what to do

thorny obsidian
#

Pixels ain't happening til Q3

thorny obsidian
cursive relic
#

Q3?

static canyon
thorny obsidian
brisk brook
cursive relic
#

Oh ok

static canyon
#

Aka Jul-Septish I think

vale ibex
#

ahhhhhhhhh

#

now alembic isn't detecting the BigInteger change for migrations

#

how annoying

clever wraith
#

I mean Pixels is open source look at our k8s cluster

brisk brook
vale ibex
#

I came here to copy my ID lol

#

was testing

#

and it worked fine

#

until I tried create a new db from migrations

#

and I don't want to hand craft a migration, since it'll just try to revert it next time I generate one

cursive relic
#

How long should it take for help channel to close?

dim pelican
#

30mins by default iirc

brisk brook
#

45?

thorny obsidian
cursive relic
#

And #help-pie closed after 12 minutes, or is my discord weird

thorny obsidian
#

If a person opens a help channel and the message is deleted, it'll close in 5-ish

cursive relic
#

Oh

vale ibex
#

it closes after 10 minutes of inactivity if the claimant hasn't typed in 30

brisk brook
#

Where'd I get 45 minutes from? Did it change somewhat recently?

thorny obsidian
cursive relic
#

Damn

thorny obsidian
#

The new thing is that we reduced it from 30 minutes since anyone has typed, to 10 minutes if not OP has typed.

brisk brook
brisk brook
thorny obsidian
#

I'm just waiting for ping-pocalypse to hit us again

cursive relic
thorny obsidian
cursive relic
#

The duration change

thorny obsidian
#

Are you saying it's a good thing or asking how it's a good thing?

cursive relic
#

Saying that it's a good thing

vale ibex
#

πŸŽ‰ All pushed

molten perch
#

Just a quick question since you used Async SQLAlchemy, do we also want to use it on the API? (It's still in beta, that's why I refrained from using it)

vale ibex
#

That's @hardy gorge's call, I know he's used async alch quite a bit

#

not sure if he has strong opinions

molten perch
#

I would keep it sync for now, tbh. Later on we can migrate to it. It's not a big deal.
(Also, I'm having problems with making pydantic accept postgresql+asyncpg πŸ˜‚ )

hardy gorge
#

The API of async sqla should be pretty stable

#

Ah, yeah, it probably expects a traditional ORM to map its attributes to

#

I like the opposite design more (domain models, where the ORM depends on the domain models via a mapper, instead of the domain models relying on the ORM), but I guess that's not easy here

molten perch
#

So, the final conclusion is to use async? I'm in the middle of the migration, I can still change it. (2 hours, tops πŸ˜„ )

clever wraith
molten perch
#

I can't seem to get it to use the async driver.
sqlalchemy.exc.InvalidRequestError: The asyncio extension requires an async driver to be used. The loaded 'psycopg2' is not async.
Any ideas? The SQLAlchemy url is in the right form. (postgresql+asyncpg)

vale ibex
molten perch
vale ibex
#

Are you using sqlalchemy.ext.asyncio.create_async_engine?

molten perch
#

Yes, I do.
Actually, it's from the env.py, but it imports the models

vale ibex
#

hmm

#

I'm guessing you switched to ```py
connectable = AsyncEngine(
engine_from_config(
config.get_section(config.config_ini_section),
prefix="sqlalchemy.",
poolclass=pool.NullPool,
future=True,
)
)

molten perch
#

What's that No, I accidentally forgot.
I'll try.

vale ibex
molten perch
#

Thank you! πŸ™‚

molten perch
#

No, actually it seems to have a problem with this piece of code, but I fail to understand what's the problem here.

    engine = create_async_engine(settings.database_url)
    session_factory = sessionmaker(bind=engine, class_=AsyncSession)
vale ibex
#

and if you print settings.database_url just before that?

molten perch
#

I tested out before, with an interactive session, and it worked.
(I switched to str in Pydantic temporarily, instead of PostgresDsn so that's not the source of the problem.. hopefully)

vale ibex
# 
from sqlalchemy.ext.asyncio import AsyncSession, create_async_engine
from sqlalchemy.future import Engine
from sqlalchemy.orm import sessionmaker

from patsy.settings import CONFIG, DATABASE

engine: Engine = create_async_engine(DATABASE.database_url, echo=CONFIG.debug, future=True)


async def get_session():
    """Yield an engine session, should used as a dependency."""
    async_session = sessionmaker(
        engine, class_=AsyncSession, expire_on_commit=False
    )
    async with async_session() as session:
        yield session
#

this is what I have currently

molten perch
#

Yeah, it's almost the same. I wonder if it's a problem that psycopg2 is installed in the env.

vale ibex
#

hmmm possible, not too sure

#

if you remove it, it might make it more obvious where the issue is

#

since it'll error sooner saying it can't find psycopg

molten perch
#

Okay, now.. it's looking for pyscopg2,
File "/app/./api/core/database/__init__.py", line 20, in <module> engine: Engine = create_async_engine(settings.database_url, future=True)
And that's the source.. πŸ˜‚

#

Nevermind, I'll try to work out something, thank you for the help! πŸ™‚

vale ibex
#

uhhhhh that's odd

molten perch
#

Problem, is that sometimes I need to just open a session when validating a model, so it's quite hard to do async there.

molten perch
#

So, I'm not sure about that. It has to be discussed. πŸ˜„
But I'm not sure how that could work for model validations. πŸ˜„

static canyon
#

RE bot#1864 is there someone that can give me a vague sort of step-by-step of what exactly needs to be done to fully implement this?

I know I'm going to need to edit the site and api repo as well, but not really sure where to start. Also, how would I go about testing as it's code from across 3 different repos.

dusky shoreBOT
molten perch
#

As for the API:
you gotta edit https://github.com/python-discord/api/blob/main/api/core/database/models/api/bot/infraction.py (Basic SQLAlchemy)
and then generate an alembic migration using alembic revision --autogenerate - m "Add DM Sent..." like a commit, then push the changes.
(you might have to edit the migration file, because of flake8, sorry for that)
If you have any questions, let me know.
(You can follow this https://github.com/python-discord/api/pull/27, basically the same thing, adding a field)

static canyon
static canyon
#

I noticed site has a similar thing for the database stuff

#

So would it be the same there?

#

I'm not familiar with SQLAlchemy so not sure what the kwargs would be

molten perch
#

yes, on the site it would be a BooleanField? Or something similar to that.

static canyon
#

It seems like I wantpy dm_sent = BooleanField( null=True, help_text="Whether a DM was sent to the user when infraction was applied." )

ashen isle
static canyon
brisk brook
#

Alright, I hope that was all PRs. If there's more needing my review feel free to request it in GitHub

#

On that note GitHub's notification system is very nice!

ashen isle
brisk brook
#

Oh you can't? I thought anyone was able to if they're the owner of the PR

ashen isle
#

i don't think i can pithink

gritty wind
#

Need write perms on the repo

#

Probably an anti spam feature

brisk brook
ashen isle
short snow
#

All messages in mod logs channel contains an embed right?

obtuse arrow
short snow
cold island
#

How is it related?

short snow
#

It fails because the log message doesn't have an embed, but since all messages in the mod logs channel have an embed, this could be ignored.

molten perch
cold island
gritty wind
#

I haven’t checked the code, but we do send non-embed text messages in there too

short snow
# cold island Why does it depend on mod log channel having embeds? Maybe I'm missing something...

For the deleted message link functionality, it would go through the latest 100 messages to find the linked message (by checking in the embed description and matching the embed format to be of deleted message). If there is no embeds then this would fail as there is error handling for this.

https://github.com/python-discord/bot/blob/32381cdfebd33b39bb69019f174b070beb32c44a/bot/exts/moderation/incidents.py#L184-L200

cold island
#

Don't make assumptions about the contents of the channel, just look for the right format

short snow
gritty wind
#

We send non-bot messages there

#

Do you filter for bot messages specifically

green oriole
short snow
#

in #mod-logs

gritty wind
#

All our log channels

cold island
#

Even if it's a bot message, if it's not in the right format you should be able to ignore it

short snow
#

nope, it doesn't... I will add the check for embeds

gritty wind
#

Yeah that’s a good idea too

#

Esp for commands

green oriole
#

ah yeah, we do write some stuff in #mod-log but it is quite rare

gritty wind
#

Tbh it can be the most rare thing and not matter

green oriole
#

yep

gritty wind
#

Checking the format shouldn’t be too difficult

short snow
stable mountainBOT
#

bot/exts/moderation/incidents.py line 189

log_embed: discord.Embed = log_entry.embeds[0]```
gritty wind
#

Make sure to validate the embed content too before you use it

short snow
#

yeah I do

#

An API endpoint for finding the mod log message for x(message ID, author ID) and action y (nickname change, message delete) can be added, it would be useful for such cases.

cold island
#

Not sure it's info we want to store. We discussed using our own message cache with persistency a while back

short snow
#

Hmm, what's the reason for not wanting to store that info?

green oriole
#

it will grow considerably large, and we have some issues on the site P99, so the less we hammer it, the better it is

shy juniper
#

Ρ‚ΡƒΡ‚ Π΅ΡΡ‚ΡŒ РусскиС? ΠΏΠΎΠΌΠΎΠ³ΠΈΡ‚Π΅ ΠΏΠΆ с ΠΏΡ€ΠΎΠ³Ρ€Π°ΠΌΠΌΠΈΡ€ΠΎΠ²Π°Π½ΠΈΠ΅ΠΌ

vocal wolf
shy juniper
#

ΠΏΠΎΠΌΠΎΠ³ΠΈ с ΠΏΡ€ΠΎΠ³Ρ€Π°ΠΌΠΌΠΈΡ€ΠΎΠ²Π°Π½ΠΈΠ΅ΠΌ ΠΏΠΆ сори

cold island
shy juniper
#

Ok sory

static canyon
brisk brook
rapid igloo
green oriole
#

CC @gritty wind @patent pivot ^

gritty wind
#

Hey Hedy, thanks for your interest, but currently we aren’t accepting outside contributions to it while we work on rewriting the docs, and opening it up

#

I’ve been meaning to do that for a while, but haven’t gotten around to it

rapid igloo
#

open source but not open to contributions, got it πŸ‘

obtuse arrow
rapid igloo
#

np, it's quite common afaik

vale ibex
#

!charinfo ## Deployment

stable mountainBOT
vale ibex
#

!charinfo

Deployment

stable mountainBOT
vale ibex
#

odd, can't see any special chars in the raw

stable mountainBOT
vale ibex
#

ah yea, it's a non-breaking space

#

github renders it as a normal space char when viewing the raw

#

had to check it out locally to get the actual raw

vale ibex
#

i mean I can't think of a reason why it would ever be intentional

short snow
#

Whats bot-core needed for?

brazen charm
#

Preventing code duplication between the bots iirc

short snow
#

Ah πŸ‘

summer garden
#

@dim pelican Do you need a hand with matching the colors to names from the json?

dim pelican
#

If that is the direction we are going it should be pretty easy to write a snippet and create the RGB: color name mapping. Just want to avoid redundant work on it

#

sir-lancebot#842 how to handle the color name matching

dusky shoreBOT
dim pelican
#

Any opinions on this?

summer garden
#

Would it be a problem to swap the hex in the json for rgb? And from there we can do as wookie suggested and get the nearest neighbor

dim pelican
#

Or, accept that there won't be a color name for every user input, even if it is close

summer garden
#

I think it's a nice utility

dim pelican
#

And if the closest match doesn't look like the original color?

summer garden
#

We can find how close it is and filter them out if it's too far

#

say 80 percent match for example

thorny obsidian
#

I would probably prefer the color name only show up if it's an exact match and if it isn't exact, don't show the name

summer garden
thorny obsidian
#

I think that's probably fine, I'm not sure how crucial having the name of a color is

green oriole
#

I would personally like to have the closest approximation

#

I have that on another bot I use for color manipulation, I find it quite useful

#

If you feel like implementing it, please do

#

Unless anyone has a strong argument against

dim pelican
#

I'm tired of working on it to be honest. Is it something that needs to be in the original PR? I feel like I've gotten the command to a "working" level about 4 times now to just have more and more things added to it.

gritty wind
#

No that's fine, you don't need to add any more if it's already working

#

Follow up PRs can be made

summer garden
green oriole
#

Yeah, let's just merge it

dim pelican
dim pelican
summer garden
#

yes. I'm not sure that including the A for alpha belongs in the error message as the "required format" but I included it in there so we could decide

dim pelican
#

Its a range from A-F is what I was trying to say

thorny obsidian
#

Yeah, I will say I think the PR has crept quite out of its original scope with a lot of these small features piling up

green oriole
#

Yeah yeah yeah, I didn't mean literal merge haha

#

Just enter go mode

fallen patrol
#

tldr feature freeze the pr

gritty wind
#

Mmm gotta love these docs

BucketType.default
The default bucket operates on a global basis.

???? wtf is a global basis lol

green oriole
#

Like

#

Do stuff

#

Track stuff

#

Globally

gritty wind
#

Thanks y'all, for your infinite wisdom

fallen patrol
#

its not very descriptive

#

!d discord.ext.commands.BucketType.default

stable mountainBOT
gritty wind
#

That makes sense, thanks

dim pelican
stable mountainBOT
#
Aye aye, cap'n!

Your reminder will arrive on <t:1636497663:F>!

ID: 3451
fallen patrol
#

er

stable mountainBOT
#

discord/ext/commands/cooldowns.py lines 240 to 242

def get_bucket(self, message: Message, current: Optional[float] = None) -> Cooldown:
    if self._type is BucketType.default:
        return self._cooldown  # type: ignore```
green oriole
cold island
stable mountainBOT
#

bot/exts/help_channels/_cog.py lines 429 to 438

claimant_id = await _caches.claimants.get(channel.id)
_unclaim_channel = self._unclaim_channel

# It could be possible that there is no claimant cached. In such case, it'd be useless and
# possibly incorrect to lock on None. Therefore, the lock is applied conditionally.
if claimant_id is not None:
    decorator = lock.lock_arg(f"{NAMESPACE}.unclaim", "claimant_id", wait=True)
    _unclaim_channel = decorator(_unclaim_channel)

return await _unclaim_channel(channel, claimant_id, closed_on)```
cold island
#

I'm having trouble understanding what's going on here. The comment says "It could be possible that there is no claimant cached", but the condition is is not None

#

And the error I got is that claimant_id ended up being None

#

Ah I see, if it's not None, then apply the lock

#

But the error still happens down the line if it's None

#

Ah I see. @vale ibex in get_or_fetch_member if member_id is None it causes a malformed API request

#

So it needs a guard before using that function

vale ibex
cold island
vale ibex
#

ahhh i read that as in the function for some reason

#

IG ```py
guild = self.bot.get_guild(constants.Guild.id)
if claimant_id and claimant := await members.get_or_fetch_member(guild, claimant_id):
await self._handle_role_change(claimant, claimant.remove_roles)
else:
log.info(f"{claimant_id} left the guild during their help session; the cooldown role won't be removed")

cold island
#

Something like that yeah

#

And change the typehint of claimant_id

vale ibex
#

yea

cold island
#

Maybe even typehint the variable outside the function where it's defined

#

Since it's not really obvious from the name

vocal prairie
#

just add the #6000

patent pivot
#

when we need to

#

depends on when I want new features

vale ibex
#

also depends what versions you are referring to. If it's things like bot, site, etc. then they're pinned to latest, so will update everytime we roll them

#

things like pg or modmail we update as needed

#

I think we're still on 8.1

#

I think there's an 8.2 now

#

afaik 8.2 doesn't add anything we need so we haven't gone out of the way to update

#

but there's also nothing stopping us

#

you can pr sure, but i don't think we'll commit to merging any time soon

#

so by the time we do, it'll likely be out of date

austere hornet
#

Lol wrong Joe

patent pivot
#

yeah I wouldn't make a habit out of it though, we bump for security or meaningful features

brazen charm
#

would appreciate a review or two on bot#1663

dusky shoreBOT
stable mountainBOT
vale ibex
#

I feel like this colour pr is dragging on a bit due to scope creep

#

How do we feel about drawing a line on features we add for now and raising issues for them?

#

So we can focus on getting some core functionality released

#

Will make reviewing additions far easier too

gritty wind
#

Yeah Brad basically said as much, that’s the current plan

#

Get it merged

vale ibex
#

Ah nice

trail pilot
#

On another note, would anyone want to review sir-lancebot#909? A quick fix (4 lines or so) for .challenge

dusky shoreBOT
rapid igloo
trail pilot
static canyon
#

The PR just needs a core dev approval and then it can be merged

gritty wind
#

(for some reason GitHub won't let me assign them so commenting instead)
They need to comment on the issue or be in the org to get assigned. I assume it's an anti-spam measure

#

Do you not have the permission to resolve comments Fronto?

#

Merged, thanks

#

.challenge R

dusky shoreBOT
gritty wind
#

Really nice

#

Do we not populate the site's OT name list in the postgres init? This is the first time I'm running the bot this late at night, so it's the first time I encounter this error.

channel_0_name, channel_1_name, channel_2_name = await bot.api_client.get(

ValueError: not enough values to unpack (expected 3, got 0)

tawdry vapor
#

I've never gotten that error and I've never had to populate off topic names manually

austere hornet
gritty wind
#

I deleted it

gritty wind
#

The bot doesn't have checks against the site API returning nothing, and the site init doesn't populate the table, so I'm not sure how you aren't getting errors

#

Are you running at midnight utc

tawdry vapor
#

No

gritty wind
#

Ah, that'd explain it. It only runs at midnight

tawdry vapor
gritty wind
#

I think Zig got that error too, and someone was PRing

#

One sec, let me find it

tawdry vapor
#

Might be related to using fakeredis and it getting some invalid cache value?

gritty wind
tawdry vapor
#

Yeah I think so. Someone else's bot made the channel claimed and then shut down, so it stayed claimed.

gritty wind
#

Right, makes sense

tawdry vapor
#

So when mine started it sees it should be dormant but it has no info for it in cache

trail pilot
trail pilot
gritty wind
#

Uhh sooo I kinda need help from someone who has nitro and is on the test server
Found a willing victim

dim pelican
#

sir-lancebot#842 should be good to go, looking for extra testers. I think Blue requested changes before the restructure, so I have re-requested a review.

dusky shoreBOT
dim pelican
#

Some previews of the current behavior are below

trail pilot
#

@dim pelican Could I join your testing server and test out some edge cases from there?
I would bring it to my local environment but I have ~7 minutes till I have to go somewhere

#

(if you don't mind)

dim pelican
#

Spam of embed photos (sorry)

short snow
dim pelican
short snow
#

yes

#

Didn't have a look at the previous ones

dim pelican
#

I'll add them really quick since its mostly cosmetic

short snow
#

can't comment on github for some reason

dim pelican
true belfry
rapid igloo
# dim pelican

This sounds a little wrong to me, it's like saying "here's some randomly selected info for this color" where I'm assuming it should be "here's some information about this randomly selected color"

dim pelican
#

@rapid igloo Better?

unknown.png
rapid igloo
#

yup

rapid igloo
brazen charm
static canyon
brazen charm
static canyon
#

I think it's just getting cut-off on your screen for some reason

brazen charm
#

shouldn't be, there's plenty of space

#

maybe I can't see that group or something

static canyon
#

Maybe?

brazen charm
#

404

fallen patrol
#

404