dev-contrib

[fossil veldt]

I can ammend an already pushed commit?

[cold island]

force push it

I usually don't like it for PRs already in review, but it's fine here

[outer oasis]

It will get rejected.
So you git push --force it in.
However, this will overwrite any and all commits that you may have changed. So, to avoid overwriting other people's work, there is also a git push --force-with-lease, which will attempt to force push, but cancel itself it sees commits from anyone else.

[fossil veldt]

huh

it's not in either merge commit?

Okay I guess that one was never merged from main

added it now

[outer oasis]

@fossil veldt 🎉🎉

[fossil veldt]

👀

!e

from pathlib import Path

Path("doggo.txt").write_text("🐕")

[stable mountain]

@fossil veldt :white_check_mark: Your 3.11 eval job has completed with return code 0.

doggo.txt

🐕

[cold island]

!e
import matplotlib.pyplot as plt
import numpy as np

t = np.arange(0, 2, 0.01)
s = 1 +np.sin(2np.pit)

fig, ax = plt.subplots()
ax.plot(t, s)
fig.savefig("output.png")

[stable mountain]

@cold island :white_check_mark: Your 3.11 eval job has completed with return code 0.

fontlist-v330.json
too long to upload

[cold island]

lol

it detected the auto-generated file

[royal prawn]

what is the autogenerated json file? i've not seen that before

[cold island]

probably some internal matplotlib thing

[fossil veldt]

hm

!e

import matplotlib.pyplot as plt
import numpy as np
import glob

t = np.arange(0, 2, 0.01)
s = 1 +np.sin(2*np.pi*t)

fig, ax = plt.subplots()
ax.plot(t, s)
fig.savefig("output.png")

[cold island]

import matplotlib.pyplot as plt
import numpy as np

t = np.arange(0, 2, 0.01)
s = 1 +np.sin(2*np.pi*t)

fig, ax = plt.subplots()
ax.plot(t, s)
fig.savefig("output.png")

[stable mountain]

@fossil veldt :white_check_mark: Your 3.11 eval job has completed with return code 0.

fontlist-v330.json
too long to upload

[fossil veldt]

!e

import matplotlib.pyplot as plt
import numpy as np
import glob

t = np.arange(0, 2, 0.01)
s = 1 +np.sin(2*np.pi*t)

fig, ax = plt.subplots()
ax.plot(t, s)
fig.savefig("output.png")

print(glob.glob("*"))

[stable mountain]

@fossil veldt :white_check_mark: Your 3.11 eval job has completed with return code 0.

['output.png', 'home', 'main.py']

fontlist-v330.json
too long to upload

[cold island]

lol wut

[fossil veldt]

does glob not return dot files?

I think matplotlib makes a .matploblib folder

[cold island]

hmmm.. does snekbox upload files recursively?

[fossil veldt]

everything under the home folder, yeah

[royal prawn]

!e

import glob
with open('.test_ionite.txt', 'w') as f:
    f.write('hello glob')
print(glob.glob('*'))

[stable mountain]

@royal prawn :white_check_mark: Your 3.11 eval job has completed with return code 0.

['main.py']

.test_ionite.txt

hello glob

[fossil veldt]

!e

import matplotlib.pyplot as plt
import numpy as np
from pathlib import Path

t = np.arange(0, 2, 0.01)
s = 1 +np.sin(2*np.pi*t)

fig, ax = plt.subplots()
ax.plot(t, s)
fig.savefig("output.png")

print(list(Path(".").rglob("**/[!_]*")))

[stable mountain]

@fossil veldt :white_check_mark: Your 3.11 eval job has completed with return code 0.

[PosixPath('output.png'), PosixPath('home'), PosixPath('main.py'), PosixPath('home/.cache'), PosixPath('home/.config'), PosixPath('home/.cache/matplotlib'), PosixPath('home/.cache/matplotlib/fontlist-v330.json'), PosixPath('home/.config/matplotlib')]

fontlist-v330.json
too long to upload

[fossil veldt]

ah

[royal prawn]

it does not apparently

[fossil veldt]

maybe we can just ignore leading dots as well then

[cold island]

hmm

why did it even create the cache in this directory

[royal prawn]

did this not happen on your test server ionite?

[fossil veldt]

it's marked as the home directory for this reason

matploblib fails if it can't write these files

[cold island]

We changed the output directory, I guess we didn't try using matplotlib since

[fossil veldt]

apparently I did not think to test matplotlib after changing the output parsing from the output folder to the main one

how do you OR a glob anti-pattern

[royal prawn]

!e

import pandas as pd

df = pd.DataFrame({'A': [1, 2], 'B': [3, 4]})
dfsty = df.style
dfsty.applymap(lambda _: 'color: red')

with open('test.html', 'w') as f:
    dfsty.to_html(f)

[stable mountain]

@royal prawn :x: Your 3.11 eval job has completed with return code 1.

001 | Traceback (most recent call last):
002 |   File "/snekbox/user_base/lib/python3.11/site-packages/pandas/compat/_optional.py", line 141, in import_optional_dependency
003 |     module = importlib.import_module(name)
004 |              ^^^^^^^^^^^^^^^^^^^^^^^^^^^^^
005 |   File "/usr/local/lib/python3.11/importlib/__init__.py", line 126, in import_module
006 |     return _bootstrap._gcd_import(name[level:], package, level)
007 |            ^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^
008 |   File "<frozen importlib._bootstrap>", line 1206, in _gcd_import
009 |   File "<frozen importlib._bootstrap>", line 1178, in _find_and_load
010 |   File "<frozen importlib._bootstrap>", line 1142, in _find_and_load_unlocked
011 | ModuleNotFoundError: No module named 'jinja2'
... (truncated - too many lines)

Full output: https://paste.pythondiscord.com/efaloyebok.txt?noredirect

[royal prawn]

can't test that feature of pandas i guess

[fossil veldt]

!e

import matplotlib.pyplot as plt
import numpy as np
from pathlib import Path

t = np.arange(0, 2, 0.01)
s = 1 +np.sin(2*np.pi*t)

fig, ax = plt.subplots()
ax.plot(t, s)
fig.savefig("output.png")

print(list(Path(".").rglob("**/[^.]*")))

[stable mountain]

@fossil veldt :white_check_mark: Your 3.11 eval job has completed with return code 0.

[PosixPath('home/.cache'), PosixPath('home/.config')]

fontlist-v330.json
too long to upload

[royal prawn]

!e

import pandas as pd
df = pd.DataFrame({'A': [1, 2], 'B': [3, 4]})
with open('test.html', 'w') as f:
    df.to_html(f)

[stable mountain]

@royal prawn :white_check_mark: Your 3.11 eval job has completed with return code 0.

[royal prawn]

is this the preferred format for HTML?

[cold island]

wdym

[fossil veldt]

I mean, normally it's not an allowed extension

we can't really render it inline either I don't think

[royal prawn]

i see

[fossil veldt]

!e

from pathlib import Path

Path(".cache").mkdir()
Path("_hidden").mkdir()
Path(".cache/abc.txt").write_text("hi")
Path("_hidden/xyz.txt").write_text("abc")

print(list(Path(".").rglob("[^._]*")))

[stable mountain]

@fossil veldt :white_check_mark: Your 3.11 eval job has completed with return code 0.

[PosixPath('_hidden'), PosixPath('.cache')]

abc.txt

hi

xyz.txt

abc

[fossil veldt]

glob doesn't support conditions on folder names..?

[stable mountain]

Missing required argument

code

[fossil veldt]

!e

from pathlib import Path

Path(".cache").mkdir()
Path("_hidden").mkdir()
Path("normal").mkdir()
Path(".cache/abc.txt").write_text("hi")
Path("_hidden/xyz.txt").write_text("abc")
Path("normal/norm.txt").write_text("0")
Path("normal/_abc.bin").write_text("0")
Path("normal/.abc.bin").write_text("0")

print(list(Path(".").rglob("**/[^._]*(/|)[^._]*")))

[stable mountain]

@fossil veldt :white_check_mark: Your 3.11 eval job has completed with return code 0.

[]

abc.txt

hi

xyz.txt

abc

[cold island]

It does, just not when it's recursive

[fossil veldt]

!e

from pathlib import Path

Path(".cache").mkdir()
Path("_hidden").mkdir()
Path("normal").mkdir()
Path(".cache/abc.txt").write_text("hi")
Path("_hidden/xyz.txt").write_text("abc")
Path("normal/norm.txt").write_text("0")
Path("normal/_abc.bin").write_text("0")
Path("normal/.abc.bin").write_text("0")

print(list(Path(".").rglob("**/[^._]*(/|)[^._]*")))

[stable mountain]

@fossil veldt :white_check_mark: Your 3.11 eval job has completed with return code 0.

[]

abc.txt

hi

xyz.txt

abc

norm.txt

0

[cold island]

** is not a specific folder's name pattern

[fossil veldt]

hm, how do I make it ignore any file / folder beginning with _ or .

[royal prawn]

i sure hope the ability to upload binary files doesn't cause any problems

[fossil veldt]

it should still adhere to the normal file extension requirements

like the whitelist we have for files per channel

oh glob.glob has include_hidden=False

but that's not in Path.glob?

[dim pelican]

Whoa is the file output live?

[cold island]

@fossil veldt I want to changelog it with an example. Should I wait for a fix?

[fossil veldt]

yeah I'm working on one

going to try to include a test as well

[fossil veldt]

maybe this

for file in self.output.rglob(pattern):
+    # Ignore hidden directories or files
+    if any(part.startswith(".") for part in file.parts):
+        log.info(f"Skipping hidden path {file!s}")
+        continue

[fossil veldt]

@cold island should be fine now with snekbox#170

[dusky shore]

GitHub

[snekbox] #170 Exclude hidden paths in files output

[cold island]

!warn 316487086746238977 Advertising is against our rules, even more so when you cross-post in several channels. Read our rules and the channel descriptions.

[stable mountain]

:incoming_envelope: :ok_hand: applied warning to @dusk blaze.

[dusk blaze]

Sorry, I thought I can share projects in those channels

[dusk blaze]

but if I made a useful project, and I want to share to with people, where I can do that in the server?

Also it's open source projects, I share them for knowledge not profit

and again, sorry for going against the rules, had no idea it's not allowed

[cold island]

Honestly, there's not a good place for it here. I would suggest the r/python subreddit. It's fine to mention your project here if it's relevant to an ongoing conversation, but posting a block of text in several channels out of context is disruptive.

[dusk blaze]

Ok

[cold island]

Hey @gleaming jay, is there any news about https://github.com/python-discord/bot/issues/2271 ?

[gleaming jay]

I'm sorry about that one, I've been sidetracked... alot.
Personal things and all that. (Making money is hard.)
But yeah, I can go do that after my nap.

[cold island]

Sure, no problem. If you end up not being able to work on it that's totally understandable, just let us know.

[cold island]

@fading galleon fwiw you don't have to keep your branch up to date with the latest commit in main. It's fine for it to go out of date as long as it doesn't affect the PR, and it avoids cluttering the git log with merges. https://github.com/python-discord/bot/pull/2446

[fading galleon]

Yeah, I know that I did that initially so I can use the new bot configuration, at the time when I first merged the main branch it had bugs with new bot configuration and the fixes for bugs kept rolling in, so if I had to use my branch I had to have the fixes, thats why I kept merging.

[cold island]

icic

[sharp crag]

@gleaming jay adding onto Zig - I can take that on if you're not able to (no rush though - take your time. Just volunteering myself as a backup just in case)

[clever wraith]

sir-lancebot#1231 needs a veeeeeeeeeery quick review in order to make sir-lancebot#1230 happen
Thank you 🙏

[dusky shore]

GitHub

[sir-lancebot] #1231 Whitelist a thread when its parent is whitelisted
[sir-lancebot] #1230 Allow .latex in python-help channel

[clever wraith]

@cold island Is this what you had in mind ?

if channels and (
            ctx.channel.id in channels or 
            (hasattr(ctx.channel, "parent_id") and getattr(ctx.channel, "parent_id") in channels)
    ):

[cold island]

No, I just meant something like

ctx_channel = ctx.channel
if hasattr(ctx_channel, "parent"):
    ctx_channel = ctx_channel.parent

And then the rest of the function works as is, it just needs to check ctx_channel instead of ctx.channel

[clever wraith]

Ah, yes yes 😅

[cold island]

@clever wraith wait 🤔

is it possible to whitelist a specific thread?

[fading galleon]

Why are we whitelisting specific threads, isn't checking if their parent is whitelisted enough?

[cold island]

We're not, I'm asking if we want to support that

[fading galleon]

IMO we shouldn't support it, I don't see a possible reason for it. Do you have a reason?

[cold island]

not really, no

[clever wraith]

You can't really know the thread's id in advance

It's created dynamically with each post, unlike our predefined channels

[cold island]

yeah I meant more generally, if we had an existing long-term thread for some reason

But it's probably not needed

[clever wraith]

It'd need to be persisted somewhere

[cold island]

yeah, in the config

[clever wraith]

Oh you mean like that ?

I thought of a more dynamic way, without having to PR it each time

[cold island]

no no

[clever wraith]

But not necessary

[cold island]

That's not what I mean

I meant, regardless of the use case, unrelated to the help forum, a possible case where we would whitelist an existing thread but not its parent

But it's too unlikely, so nevermind

[clever wraith]

Oki boss

[fading galleon]

Also Zig you remember the issue about pasting large amounts of code in #python-discussion I checked and there are 2.4k+ instances of that embed being sent in general, so why don't we automate it?

[clever wraith]

I pushed the change

[cold island]

no, I don't 😅 a reminder?

[fading galleon]

ah it was bot#439

[dusky shore]

GitHub

[bot] #439 Auto suggest the use of pydis hastebin on longer code messages

[fading galleon]

we had a discussion on this here

also found that #dev-contrib message

[fading galleon]

So your thoughts on this data Zig?

[cold island]

2.4k is not that impressive when you factor in the volume of total messages in that channel 😄
I'm not too opposed to trialing it, but I'm still a bit on the fence. At the very least the threshold would need to be sufficiently high so that it's not too noisy.

[fading galleon]

Its impressive when you look at the number of messages sent by the bot in that channel which is almost 20k+ so 12-13% messages in pygen from the bot is that embed.

[cold island]

why does the number of messages from the bot matter?

[fading galleon]

It matters because it shows how many times people have used the bot in pygen

[cold island]

ok, but it's not a metric of how common it is in general, it's a metric of how common it is out of all commands used. Not sure it's the right one

[clever wraith]

Also, python-general is not particularly meant to discuss big chunks of code, the pace in that channel is too fast to have the time to process that code, test it, iterate over it, and whatnot.

So it shouldn't count as a metric to use the bot for that in pygen IMO

[cold island]

Btw Python has 107k messages there, Discord search sometimes doesn't display all results

[fading galleon]

It shows 400k now lol

[fading galleon]

We could add a high timer for it as Zig said?

Also those 2.4k times I guess large chunks of code have been posted thats why people invoked the command

We should have a high limit for line numbers like 75+ and 240 seconds timeout.

If my suggestions don't seem fine atleast we should close the issue.

What do you guys think? @cold island and @clever wraith

[cold island]

I'll think about it a bit

[fading galleon]

Thanks for thinking

[tawdry vapor]

@fossil veldt sorry for not following up on a more timely manner. I wanted to express that it seems redundant to default to excluding underscore prefixed files if we're also going to exclude dot prefixed files.

It's a bit unintuitive that dot files are ignored using a different mechanism than that glob pattern.

In retrospect I think the ideal thing to do would have been to exclude dot files and not even add a feature to configure a glob exclusion.

[fossil veldt]

yeah I agree it's a bit confusing. Mainly at first I wanted the underscore exclusion to make it possible to write to an importable python file and not have it uploaded

maybe we can make both conditionals and get rid of the glob exclusion? Or they could be bool arguments

exclude_dot=True
exclude_underscore=True

[tawdry vapor]

That's a good point. Fair enough.

Maybe it's better to change the glob to regex? It's strange that it currently does not apply the exclusion recursively.

[fossil veldt]

yeah it seems glob cannot actually apply any rules on recursive folders? Only the ** is special cased as "recursive folders"

oh

hm

Path.glob seems to be more restrictive than glob.glob, very strange

!e

from glob import glob

print(glob("[!.]**/[!.]*"))

[stable mountain]

@fossil veldt :white_check_mark: Your 3.11 eval job has completed with return code 0.

[]

[fossil veldt]

!e

from pathlib import Path

print(*Path(".").glob("[!.]**/[!.]*"))

[stable mountain]

@fossil veldt :x: Your 3.11 eval job has completed with return code 1.

001 | Traceback (most recent call last):
002 |   File "/home/main.py", line 3, in <module>
003 |     print(*Path(".").glob("[!.]**/[!.]*"))
004 |   File "/usr/local/lib/python3.11/pathlib.py", line 952, in glob
005 |     selector = _make_selector(tuple(pattern_parts), self._flavour)
006 |                ^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^
007 |   File "/usr/local/lib/python3.11/pathlib.py", line 289, in _make_selector
008 |     raise ValueError("Invalid pattern: '**' can only be an entire path component")
009 | ValueError: Invalid pattern: '**' can only be an entire path component

[fallen patrol]

yes

[fossil veldt]

but... why?

[fallen patrol]

https://github.com/python/cpython/blob/v3.11.2/Lib/pathlib.py#L940-L954
https://github.com/python/cpython/blob/v3.11.2/Lib/glob.py#L13-L29

there's the sources

[fossil veldt]

I mean like, shouldn't it?

is it not feasible for Path.glob to support the same features as glob.glob

[tawdry vapor]

I don't know. But if we can fix this by switching to glob.glob then that's even better

We could then use that pattern to exclude dot files as well, correct?

[fallen patrol]

https://github.com/python/cpython/blob/878ead1ac1651965126322c1b3d124faf5484dc6/Lib/glob.py#L17-L20

[stable mountain]

Lib/glob.py lines 17 to 20

The pattern may contain simple shell-style wildcards a la
fnmatch. Unlike fnmatch, filenames starting with a
dot are special cases that are not matched by '*' and '?'
patterns by default.```

[fallen patrol]

automatically excludes them

[tawdry vapor]

So that does what we want out of the box?

[fossil veldt]

I attempted that but it seems it did something different than Path, it was strange

like this change failed multiple tests for some reason

for file in self.output.rglob(pattern):
  ...

files = glob.iglob(pattern, recursive=True, root_dir=str(self.output))
for file in map(Path, files):
  ...

[fallen patrol]

are there any breaking changes to snekbox with the new bytes output?

[tawdry vapor]

Not to the API

[fossil veldt]

for the web API no

[fallen patrol]

so also the expected output shouldn't change?

[fossil veldt]

the response has a new key files

[fallen patrol]

i need to harden my changes to snekbox so i can potentially pr them upstream

would be the easiest change

[tawdry vapor]

Would like to pursue this further but it's not urgent. Would appreciate if you opened an issue for this. The main goal is to get the exclusion working recursively. Hopefully we can do that by continuing to accept the same kinda of glob patterns.

[dull swan]

I assume you're all aware of the sequence break in Pybot. Bringing it up here just to make very sure it's not something that someone noticed and didn't say anything about.

[cold island]

sequence break?

What happened to the song?!

[cold island]

oh yeah there's an open issue

[dull swan]

Sorry I ran off to make food after I posted that. All good, just making sure 😛

[cold island]

thanks for checking

[crude gyro]

ehh. it sucked and I don't wanna pay for extra SoundCloud space so I keep deprecating old crap.

didn't think anyone would notice

[sharp crag]

narrator: they did indeed, notice

[royal prawn]

as @full fractal likely *re-*discovered in pygen a bit ago, pybot does not like when rule numbers are followed by other (non-whitespace?) characters. is this acceptable behavior, or a bug?

!rule 1 (is fine, but)

[stable mountain]

Rules

1. Follow the Python Discord Code of Conduct.

[royal prawn]

!rule 1.

[stable mountain]

Rules

The rules and guidelines that apply to this community can be found on our rules page. We expect all members of the community to have read and understood these.

[tawdry vapor]

I don't consider this a bug

[full fractal]

It's incredibly marginal. I don't think this is worth addressing.

[clever wraith]

@rapid swallow the appeals2 category has been renamed to appeals_2

[hoary haven]

fyi ive seen another 2 reports of the #roles button not working initially

[clever wraith]

Does restarting the client work ?

[gritty wind]

I think I know what this is

[hoary haven]

for reporter #1 (which is actually me, on a different account) a refresh fixed it. for reporter #2 it didn't, but i can't verify that

[gritty wind]

Are we storing the user’s ID as a class attribute

[hoary haven]

but they were able to add one of the roles a bit later

[gritty wind]

!src roles

[stable mountain]

Command: roles

Returns a list of all roles and their corresponding IDs.

Source Code

Go to GitHub

bot/exts/info/information.py:115

[royal prawn]

speaking of restarting fixing things, has there been progress made for why that sometimes fixes the apps right-click menu not showing up, or why it goes away in the first place?

[hoary haven]

I'd be surprised if that weren't a discord bug

[royal prawn]

that we can't even try to debug or fix. yay

[hoary haven]

I'm also pulling this out of my butt and may be wrong

[gritty wind]

Is someone free to help do a bit of testing right now for the roles thing

[gritty wind]

I think you're probably right

[clever wraith]

yalla

I am

[gritty wind]

Could you click on the button in roles, but not try to do any role changes. I'll give you a signal to click on them, just don't leave that channel

You might want push notifictions on for that, I'll ping you

[clever wraith]

signal me with a dm

[gritty wind]

Good idea

[clever wraith]

ok, i'll click in 5 seconds

[gritty wind]

👌

I can’t actually DM you lol

@clever wraith you can come back now

[clever wraith]

Ahahahaha I opened mobile in //

Reproduced

[gritty wind]

Alright I know the issue

[royal prawn]

tried this myself just now. aoc and lovefest worked fine, complaining about the time of year, but roc failed. fixed by client restart

[gritty wind]

Since the user is a class attribute, whoever opens the interaction last has priority

Essentially kicking everyone else off

https://github.com/python-discord/bot/blob/main/bot/exts/info/subscribe.py#L74

[stable mountain]

bot/exts/info/subscribe.py line 74

interaction_owner: discord.Member```

[gritty wind]

Despite this being set in the init, it still propagates to other instances

[clever wraith]

but wait

[clever wraith]

ah, that was gonna be my question

[gritty wind]

We'll need to investigate further, but hey we now have a repro

[royal prawn]

so is that just switch to instance variable only to fix perhaps?

[clever wraith]

Do you want to test this in my server ?

[gritty wind]

If you're looking for a test dummy, yeah sure

Can't get to code rn tho

[clever wraith]

Yes yes, I'll make the change & run my bot

[fading galleon]

bot#2439 and bot#2436 needs to be merged before I start working on error handlers for interaction

[dusky shore]

GitHub

[bot] #2439 Make help showable through button on command error message.
[bot] #2436 Centralise error handling for commands invoked from error handler

[clever wraith]

we found it

It's most probably a design flaw

I'll have a look at the docs later on to see how discordpy recommends doing this

[clever wraith]

!remind 2h review

[stable mountain]

I got you.

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

ID: 5728

[clever wraith]

Can't look now, I'll try to get to them later

[fading galleon]

Yea I will also review them after my lunch

[gritty wind]

Whatever d.py is doing under the hood, it calls the check from an incorrect view instance (only the latest one), therefore self.author becomes incorrect, and fails the check

Why it does that, and whether this is a usage bug or lib bug is a bit unclear

But does anyone know why we actually do this check at all

I'd think it'd be fine to just always return true

Anyway, once we do fix this, this will actually present a second issue

[solemn laurel]

please help me in #discord-bots

[gritty wind]

The text in the button (remove/add) will be a combination of all the interacctions

[gritty wind]

Wrong channel, just post your question there and wait

[gritty wind]

I.e: users A and B start with no roles
User A triggers the interaction
User B triggers the interaction

B adds role 1
Text is updated to "Remove Role 1"

A adds role 2
Text is updated to "Remove Role 2", but also, they will see "Remove Role 1"

I think we should be checking and updating the interaction based on user IDs, and checking which role operation to perform based on the user's current roles, instead of assuming that the class instance is going to be unique for any given user, since that does not appear to be a correct assumption

cc @clever wraith

[stable mountain]

It has arrived!

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

[clever wraith]

I think it was because the views weren't ephemeral before, so we didn't want people to add roles for others

Not sure of course, just a theory

[gritty wind]

Before views it was a text command I’m pretty sure

!subscribe

That didn’t bring up a view before

[clever wraith]

bot#2341 should be the one where the change happened

[dusky shore]

GitHub

[bot] #2341 Allow members to self assign roles through a persistent view

[clever wraith]

But it was a view before as well in that PR

[rapid swallow]

haha I noticed

thought I misspelled it but then git blame'd you

wow the metaclass black magic has been removed

[clever wraith]

Weops

[rapid swallow]

hurray?

[clever wraith]

Hurray !

[clever wraith]

It's still black magic

You just don't see it xD

[rapid swallow]

I havent checked your PR yet

once I do I'll come after you

[clever wraith]

🧙

[fading galleon]

Sugesstions on these guys please
https://github.com/python-discord/bot/pull/2446#discussion_r1130964306
https://github.com/python-discord/bot/pull/2446#discussion_r1130945854

[clever wraith]

@gritty wind is there a reason why _select_lock_channel is async here ?
https://github.com/python-discord/bot/blob/e21cd3ecea9afc7cf5d08bf75c944dd6049f4d28/bot/exts/moderation/silence.py#L95

[stable mountain]

bot/exts/moderation/silence.py line 95

async def _select_lock_channel(args: OrderedDict[str, any]) -> TextOrVoiceChannel:```

[clever wraith]

If you still remember ofc ahahha

[vale ibex]

iirc it's because our lock decorator always awaits the lock func

[clever wraith]

There are two execution paths

[vale ibex]

or at least did at some point in time

[clever wraith]

if it's awaitable, await it

otherwise call it

I was curious to know, as it doesn't quite seem to be necessary

But doesn't hurt anyways :B

[vale ibex]

yea, it's not needed now

[fading galleon]

Chris or anyone, what do you think about this, for the source slash command's autocompleter should we start showing app_commands.Group and app_commands.ContextMenu also? Currently it only shows app_commands.Command

[rapid swallow]

@clever wraith botstrap is sooo cool

so satisfying to see .env.server at the end

the only thing I feel it's missing is a success message at the end

[clever wraith]

I'm very glad you like it!

[clever wraith]

That's a great addition, do you want to add it yourself?

[rapid swallow]

I can try

I'll be travelling in the next few hours though

[rapid swallow]

what kinda message should I add though? the number of role IDs, category IDs and channel IDs added? Not sure how useful it will be but it will look cool .

Or a simple "\nScript has finished running!" or something of similar effect?

[clever wraith]

We could do that, even though there are logs of channels / roles that couldn't be mapped .

But yes, a finishing log with a duck (if there's Unicode for that) will be cool ahahah

[cold island]

A summary sounds useful, to say how many things were created/updated/removed in every section

Not high priority though

[outer oasis]

@fossil veldt thoughts on bot#2464 & snekbox#172 ?

[dusky shore]

GitHub

[bot] #2464 BOT-3EQ: long file extensions cause !eval result responses to breach the message length limits
[snekbox] #172 BOT-3EN: snekbox hits a recusion exceded exception in pathlib when parsing lots of nested directories and 500s

[outer oasis]

Both admittedly pretty esoteric though

[fossil veldt]

is there snekbox logs for 172

[outer oasis]

Yeah the link in issue will.... Do you have access?
Let me see if I can export

[fossil veldt]

[outer oasis]

Oh yeah that too
Snekbox logs aren't there though

[fossil veldt]

grafana says this

:(

[outer oasis]

https://paste.pythondiscord.com/conazokeka
well that's a hecking mess

[fossil veldt]

ah

I didn't think the file scan could actually fail

it's already time limited

is this pathlib limit even documented

nice

[outer oasis]

Oof
Where'd those come from?

[fossil veldt]

pathlib docs page

[outer oasis]

docs.python.org has a dark mode?

OH

[fossil veldt]

[outer oasis]

That's a search

[fossil veldt]

[outer oasis]

ic ic ic

[fossil veldt]

um...

so basically just avoid pathlib from now on...?

[outer oasis]

Hecking heresy!!

[outer oasis]

Oh
Oof

[fossil veldt]

oh it finished

0.5s vs 87s

[outer oasis]

[fossil veldt]

https://github.com/python-discord/snekbox/actions/runs/4395009986/jobs/7696475837

@tawdry vapor so the new test on Ubuntu 22.04 has been running for 3 hours now while the 20 one passed.

I have no idea what's going on, any ideas

it seems there is some bug in multiprocessing pool that can make map async end up in a deadlock loop forever?

not sure if there's a better way to do the time limits for file parsing

or we could have a total / per file size limit

[clever wraith]

@hoary haven @gritty wind here we go bot#2465

[dusky shore]

GitHub

[bot] #2465 Fix broken roles view

[clever wraith]

The culprit was the custom_id given to the roles/view

It persists the view ...

That's why my interactions go to the latest created view ..

@cold island Should we keep the text command still ?

Let me check its usage first i guess

Well, discord search is not gonna make it easy for me

[cold island]

oh it was given a custom ID? that explains it

don't see a reason to remove the text command for now, I guess we can keep it

[clever wraith]

Sure sure

[clever wraith]

Such an easy thing to miss right? Ahaha

Drove me crazy 😛

[fading galleon]

If anyone is looking to do something bot#2439 and bot#2436 need reviews, they are currently blocking issues bot#2022 and bot#2424 being fixed by bot#2446

[dusky shore]

GitHub

[bot] #2439 Make help showable through button on command error message.
[bot] #2436 Centralise error handling for commands invoked from error handler
[bot] #2022 Incorrect handling of tags by the !source command
[bot] #2424 Make !source a slash command
[bot] #2446 Migrate !source to /source (app_commands)

[tawdry vapor]

No idea, sorry.

[fossil veldt]

@outer oasis btw will ERROR level logs automatically have a sentry report? Or are those only for crashes

[outer oasis]

Uh....

Each thing is configured slightly differently
Like Snekbox had ERROR logs and then crashed and burned and died, and... didn't make a Sentry issue at all

But in general, logs just log, they won't trigger events in things like Sentry.
And Sentry, uh-- Sentry gets unhandled exceptions
Sentry doesn't get a new CommandNotFound every time someone makes a typo

[cold island]

this is because of the error handler nevermind I see your point

Generally anything above info should make a sentry issue if the project is configured to do so

so warn, error, exception

The Python bot is, not sure about snekbox

[gritty wind]

Sentry does get those errors

It’s just that if it has seen the error before and it’s still open it’ll group that error with previous instances and not send a new alert

Does snekbox actually use sentry

[outer oasis]

It's on there, but I set it all the way back to "90 days" (as far as it would let me go), and it didn't show me anything
Was just blank

[gritty wind]

I don’t see it being init anywhere, despite being installed

Oh it’s in init

[outer oasis]

Where?

So... if it's there, then why didn't snekbox#172 create a record??

[dusky shore]

GitHub

[snekbox] #172 BOT-3EN: snekbox hits a recusion exceded exception in pathlib when parsing lots of nested directories and 500s

[outer oasis]

Just blank for snekbox
Nothing at all

[gritty wind]

That’s the part I don’t know

I’m guessing we aren’t properly having sentry wrap the application

[tawdry vapor]

It's picking up new releases, for what that's worth

It only has falcon integration. Maybe it also needs logging integration. I'm not sure what coverage the falcon integration has

https://github.com/getsentry/sentry-python/issues/643

A PR that supposedly fixed this has been merged though

https://github.com/getsentry/sentry-python/pull/1733

Might just need to update sentry in our deps

Yeah we need to change the constraint to be at least 1.16.0

Hopefully that will fix it?

Currently it's locked to 1.10

[outer oasis]

Let's see....
https://github.com/python-discord/snekbox/blob/main/pyproject.toml#L36
https://github.com/getsentry/sentry-python/blob/master/setup.py#L24

[stable mountain]

pyproject.toml line 36

sentry = ["sentry-sdk[falcon]>=1.5.4"]```
`setup.py` line 24
```py
version="1.16.0",```

[outer oasis]

Yeah we're a bit behind

[outer oasis]

Oh you already found the specific version

[tawdry vapor]

That's the minimum version but we're actually on 1.10 if you look at requirements.pip

[tawdry vapor]

Yeah GitHub has a useful feature that shows all the tags a commit is present in. So I just clicked the commit from that last PR linked

Would appreciate someone opening a PR to bump the lower bound version constraint to 1.16.0 for sentry and then running pip compile (there's a make command for it).

I wonder if this can be removed once we update  https://github.com/python-discord/snekbox/blob/main/snekbox/utils/logging.py#L27-L28

[stable mountain]

snekbox/utils/logging.py lines 27 to 28

with warnings.catch_warnings():
    warnings.filterwarnings("ignore", message=r".*\bapi_helpers\b", category=DeprecatedWarning)```

[outer oasis]

snekbox#174

[dusky shore]

GitHub

[snekbox] #174 Update dependency versions

[clever wraith]

Is there a reason we don't have dependabot setup for snekbox ?

[tawdry vapor]

Just haven't bothered. But also I doubt it supports pip compile, at least the way it's set up for snekbox

[clever wraith]

What makes its setup special for snekbox?

[tawdry vapor]

There different files for different sets of dependencies

I haven't looked into it. Maybe it can be configured to work with that

I'm not totally sold that snekbox really needs dependabot besides addressing security issues or deprecations

[clever wraith]

Aren't both of those a good reason?
I know we might not need new features that come with these updates.

Is it because of the potentially unnecessary testing that comes with it and the time spent in checking change logs to see if everyone is respecting semver or not? 😛

[tawdry vapor]

Yeah they are. What I'm trying to say is that i don't think we need to update immediately for every minor version release. Would rather keep things stable and only update if we really need to.

Though I think we can be more confident in its test suite compared to our other projects.

I also just think it's really noisy. Both with the amount of PRs created and the commit history. Yeah there are ways to exclude certain authors from git log but eh...

Maybe this sort of thinking is how software builds technical debt and makes it difficult to migrate to newer versions. But we only have two core dependencies (Falcon and gunicorn) so I think we can manage to stay on top major updates ourselves

[clever wraith]

Sure sure, I understand.
I guess the security vulnerabilities are contained thanks to NsJail so we're fine with that.

As for the deprecations and based on what you're saying, it's not something that happens quite often that it breaks something in snekbox.

I just thought that we'd want to avoid such cases.

As for the log history and the noisiness, I don't fully see that as a reason to hinder updates for a project, it's one of the '' trade-offs '' let's say to reduce technical debt and issues

Ahahah that was my point

Sure, if it's only 2 core ones and we want to montoir them ourselves then it's not much of an issue, and I've been here for 1.5 years now and i feel this is the first time I've seen a dependency related issue with snekbox 😛

(my engagement wasn't the same 1.5 years ago, but it's just a feeling :P)

[tawdry vapor]

This issue was actually due to a gap in test coverage (though not sure how realistic testing sentry integration would be). Ideally would've been caught when the update to falcon 3 was done

[tawdry vapor]

I can look into it. Maybe a bot could be helpful if we could configure it to only notify for updates we care about

[gritty wind]

Sentry provides a test integration thing which just intercepts all outgoing data and allows you to verify it works in tests

But it’s not super important since that part of our system is managed by falcon

So as long as falcon stays up to date

The technical debt issue is the main motivator to use dependabot, but I don’t think it supports pip compile either

Given the maintenance and update states of snekbox, I think that’s one of the projects it’s reasonable to ignore in

[clever wraith]

Agreed 😄

[sharp crag]

So, do I have to make seperate issues for the some of the tag PRs I'd like to open for site#295 ?

[dusky shore]

GitHub

[site] #295 organize tests of all apps into one directory

[sharp crag]

Sorry, site#695

[dusky shore]

GitHub

[site] #695 Migrating #discord-bots pins to the site

[sharp crag]

These ones

I'm wondering if I just just make a PR directly and link to 695 instead of having to make duplicate issues for all of them covered in the 695?

[outer oasis]

yes

"updates as part of 695"

[sharp crag]

Ah, does that show up as a "milestone" on github?

[outer oasis]

No

[sharp crag]

Would've been nice. No matter though, just wanted to know the policy with issues before I continued 👍 Thanks

[outer oasis]

"Milestones" are specific things that you can create
https://docs.github.com/en/issues/using-labels-and-milestones-to-track-work/about-milestones

If you make checkboxes you get the "x of x tasks" on your issue

[outer oasis]

Look at kubernetes#140

[dusky shore]

GitHub

[kubernetes] #140 Remove REPO_TOKEN PAT from kubernetes deploy workflows

[static canyon]

bot-core#137 bot-core#141 sir-lancebot#1229 are all in need of reviews. The last one is just a small bug fix. Reviews appreciated 😄

[dusky shore]

GitHub

[bot-core] #137 Add check_rerun_command util to commands.py
[bot-core] #141 Migrate all message utils from bot and sir-lancebot.
[sir-lancebot] #1229 Correctly handle when the movie API doesn't return any movies

[clever wraith]

I'm curious, you mentioned that the eval command had flaws in it in, but you only gave the example where you specify the python version upon rerun .

What are the other flaws you were talking about ?

I'm asking because I want to make sure I address them when testing

Would also be useful to include them in the PR as that would help anyone without context to know what to look for especially aside from the regular testing they'd bee doing

[static canyon]

The bug is that when you re-run, everything in the message other than the !e is treated as code

So in the code of specifying the python version, the python version is treated as code

Or if in the future some other parameter is added then that would be treated as code too

I don't think there's anything else
Actually from the wording it does definitely sound like there's another, but I don't remember it

Tbf it was 6 months ago now

I'll take a look when I get a chance

!e 3.10 py print("Hello, World!")

[stable mountain]

Sorry, an unexpected error occurred. Please let us know!

ClientResponseError: 400, message='Bad Request', url=URL('http://snekbox-310.default.svc.cluster.local/eval')

[clever wraith]

Snekbox 3.10 is currently broken

[static canyon]

Eh, yeah

[clever wraith]

So that won't eval

[static canyon]

I was thinking I could still edit to re-run but I guess not

[clever wraith]

you can still specify 3.11 ig, no ?

[static canyon]

But yeah, when you go to rerun it'll execute 3.10 print("hello world") which will then cause a syntax error

[clever wraith]

but i get the point

[static canyon]

!e 3.11 py print("Hello there!")

[stable mountain]

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

Hello there!

[static canyon]

Eh, idk

!e

[stable mountain]

Missing required argument

code

[static canyon]

!src e

[stable mountain]

Command: eval

Run Python code and get the results.

Source Code

Go to GitHub

bot/exts/utils/snekbox/_cog.py:546

[clever wraith]

they need to be inline if you want to reproduce i suppose

[static canyon]

!e 3.11 print("Hello")

[stable mountain]

@static canyon :x: Your 3.11 eval job has completed with return code 1.

001 |   File "/home/main.py", line 1
002 |     3.11 print("Hello")
003 |          ^^^^^
004 | SyntaxError: invalid syntax

[static canyon]

Yeah, there we go

[clever wraith]

since having it with backticks gets into a different exec path

yeah

[static canyon]

Yep

Unfortunately if there was another bug I can't remember it

[fossil veldt]

what's up with this? I was able to run both fine locally

or are we just not hosting 3.10

[outer oasis]

I believe it's because your files PR caused CI to deploy the updated version to the main deployment, but not to the alternate deployment that's hosting 3.10

[outer oasis]

No it's still there
It's throwing 400(s)

[fossil veldt]

oh huh

[static canyon]

It was deliberately removed since 3.10 and 3.11 are basically the same

There was a comment from scaleios in #community-meta

[fossil veldt]

I didn't change CI though

[static canyon]

#community-meta message

[outer oasis]

CI did it's job
It updated the deployment

When we created an alternate deployment to run a different version, we didn't add CI for that

[outer oasis]

Oh right I forgot

[fossil veldt]

in any case we really should just update snekbox to support multiple interpreters

[outer oasis]

Yeah, we'd rather implement something like snekbox#158, instead of trying to deploy multiple verisons of snekbox simultaneously

[dusky shore]

GitHub

[snekbox] #158 Allow Evaluation Using Multiple Python Versions

[outer oasis]

Jinx

[fossil veldt]

running 2 server endpoints for version support is not ideal

[outer oasis]

Something you'd be interested in working on?

Actually doesn't sound like much effort just to install multiple versions
But it does sound like a bunch of effort to install packages for each of them
Because right now we pin exact versions, don't we? And support for versions wouldn't be the same across versions

[cold island]

I think @gritty wind had something in mind

[fossil veldt]

?

[fossil veldt]

I think most of our pinned versions also support 3.10 though

but yeah if we wanted to go down to 3.7/3.8 maybe some are incompatible

[outer oasis]

How many versions do we want to install?

All supported versions + beta + alpha could be cool. But... How much maintenace is that?
Is it literally just pyenv install $version; pyenv $version -m pip install packages-$version.txt? (assuming we use pyenv)

[fossil veldt]

maybe all bug fix versions to start

so 3.8-3.11

[outer oasis]

All the dot releases? That would be HUG-

... oh

[fossil veldt]

or perhaps even 3.12 alpha

we could attempt to install packages one by one and ignore fails

[outer oasis]

Like this?

[fossil veldt]

yeah

[outer oasis]

ic ic

[fossil veldt]

since there'd be a point if someone discovers some bugged behavior and it'd be eligible for a fix

[outer oasis]

Fair

[fossil veldt]

though maybe we could do 3.7 as well

to have the security ones

3.7-3.12

[outer oasis]

It would be neat to have everything

But just for container size and ongoing maintenance burden, I'm thinking that many versions would be a bit much

... How big is an installation of Python?

[fossil veldt]

storage can't be that much of an issue right?

[outer oasis]

We’re not going to, like, run out of space or anything.

Just the bigger the container gets the more time it’s going to take to build/upload/download

[vale ibex]

I'd also like to understand the the scale of the difference. If we're suddenly going to make the container 5x larger, it does make local dev much more prohibitive

[outer oasis]

Could also still have a different manifest with only one or two versions for smaller builds for local dev or testing or whatever
And just have CI build a final giant container with lots of versions

[vale ibex]

Yea, having that option would be good if it is a huge image

if it's not that much of a size change, likely not worth the effort

[outer oasis]

Also fair

[gritty wind]

It’s kinda stupid to literally have every version from 3.7 to 3.12, it doesn’t offer us anything

Ok so to addresss a few things

The reason 3.10 is broken is because of the file PR. Your feature set ionite is “backward compatible”, but the previously deployed version of snekbox is not “forward compatible” with things that don’t exist yet. Since bot now sends an extra arg in the request, the 3.10 server which is on an old build of snekbox does not work. The reason it wasn’t built by CI or w/e is because that CI does not actually exist. When we upgraded to 3.11, we published one final 3.10 container by hand, and I opened the snekbox issue that has been linked to, but it never really got a response

In terms of issue implementation, I did start working on it this week to provide a poc, but it’s not my top priority and other things have taken priority

I’ll get to it on the weekend

In terms of version size, it takes anywhere from 100mb-750mb per python version depending on how it’s installed and before dependencies. It also takes a fuck-ton amount of time depending on how it’s installed, and once again before dependencies

We can easily publish 2 versions in snekbox, a full and lightweight version with 2 different sets of python interpreters, that’s part of what I’m working on

[gritty wind]

While I continue working on this, the summary of my work is I created a script which sets the correct version information throughout the codebase. In docker, I've opted for using the pre-existing images to save on size and build time, and here's what the script does:

It takes in a JSON file, which looks a little like:

{
  version_name : 3.11
  version_tag : 3.11-slim-buster
},
{ // -- 3.10 -- // },
...

So if you specified 3.9, 3.10, 3.11 in that order, and specified that 3.11 is the "main" version (i.e the version which actually runs the server code, you'd get the following dockerfile generated:

# ------------------------------------------------------------------------------
FROM python:3.11-slim-buster as base-first
FROM python:3.9-slim-buster as base-3-9
COPY --from=base-first / /
FROM python:3.10-slim-buster as base-3-10
COPY --from=base-3-9 / /

# ------------------------------------------------------------------------------
FROM python:3.11-slim-buster as base

COPY --from=3-10 / /

... # From here on out, the dockerfile is unmodified

Now in that final base layer, python3.9, python3.10, python3.11 are all valid commands, and python and python3 are just 3.11

The final build size before packages for this approach is 640mb for the 3.9,3.10,3.11

The dev container before was 451 MB

(~100mb per version)

[gritty wind]

@tawdry vapor I need to re-generate the protobuf config

It seems it was last modified in snekbox#127

[dusky shore]

GitHub

[snekbox] #127 Add cgroupv2 support

[gritty wind]

What version did you build with?

This was like 2 years ago dw if you don't know

[tawdry vapor]

It is the same as the version of nsjail in the docker image

And that should always be the case

[fading galleon]

@clever wraith I think you might be looking for artifact for .github#17 https://github.com/SebastiaanZ/github-status-embed-for-discord#reporting-the-status-of-a-pull_request-triggered-workflow

[dusky shore]

GitHub

[.github] #17 Add a reusable status_embed workflow

[clever wraith]

No i'm actually trying to avoid it

[fading galleon]

oh

but why?

[clever wraith]

Because triggers are per repository.
I can't have the status_embed in the .github repo & listen for completed run events from other repos.
The only way to do that, AFAIK, is by reusable workflows.

The point was to have something more modular & pass as less info as possible, but I don't want to pack things, upload them, download & unpack them.

I had hopes it would work with proper reusable worklows, and it can, just not for our case since it involves some security vulnerabilities since we need a special event for that that gives access to our repos context, which is not what we want when code execution is involved

[fading galleon]

Oh, alright

[gritty wind]

@fossil veldt @cold island got it done :)
PR is snekbox#175, turned out pretty easy

[dusky shore]

GitHub

[snekbox] #175 Support Multiple Python Versions

[gritty wind]

I wish discord didn't fuck up my images like that

[fossil veldt]

👍 nice thanks

[fossil veldt]

looks good mostly, would it be better to just have the autogenerated mount part for snekbox.cfg to just be inlined as a dynamic argument?

that avoids having to write to the cfg and also only mounts the requested version

[gritty wind]

The main I reason I avoided it was I wasn't sure if it has any performance overhead or not

Not sure that makes sense even

I'm open to changing that yeah, but right now my main focus is just getting people to sign off on the idea so I can finish up the PR

[fossil veldt]

hm, yeah perhaps

how large is the image with 3.10+3.11?

[gritty wind]

Uhhh I don't have that number on me, but 3.9+3.10+3.11 is 640 to prod's 450

[fossil veldt]

not too bad I suppose

[gritty wind]

Part of the idea here will also be to upload two images to registery

[fossil veldt]

we were running 2 servers before, so it's better at least

[gritty wind]

a dev version with just 3.11 and the main one with all versions

For prod the size is not actually a concern

It's mostly a concern for people having to download the image to work on other projects

Pushed your suggested change, seems fine. Removing 3.9 takes us down to 600

[fossil veldt]

do we want to support 3.8/3.9 for here as well?

imo it's pretty useful when trying to show cpython bugs but I don't see much use outside of that

[gritty wind]

I'm thinking 3.9, 3.11, 3.12b as the main version we'd have

[fossil veldt]

3.12 would be nice

[gritty wind]

I guess there is no explicit downside to supporting more versions now

We'd probably change the eval selector to a dropdown which would let us have as many versions as we want

[fossil veldt]

It's still alpha now though, 3.12.0a6 was released today

[gritty wind]

Not sure how to get the packages installed on all python versions without essentially hardcoding a bunch of things tho

Our current approach is fine if you only have to track package versions for one python version

[fossil veldt]

so currently we share the installs between 3.10 and 3.11?

[gritty wind]

Nah currently they are separate in the PR

Which is probably ideal, but managing 10 packages across four python versions is not fun

[fossil veldt]

wait so how does our old deployment.yaml still work

[gritty wind]

This PR isn't actually finished yet, I'm just showing off the idea

But the deployment would actually still work because

It uses pip install ... which will default to whatever pip is on top of PATH, which is the main version's

So it will install the deps for python 3.11, and nothing for 3.10 if we ran it

[fossil veldt]

do we have to specify a separate PYTHONUSERBASE?

[gritty wind]

Python handles that

[fossil veldt]

oh huh

[gritty wind]

If you specify PYTHONUSERBASE=/snekbox/userbase, python3.11 would install to /snekbox/userbase/lib/python3.11 etc

[fossil veldt]

we could make our own dependency installer that parses something like poetry's specifier I guess 🥴

[tool.poetry.dependencies]
foo = [
    {version = "<=1.9", python = ">=3.6,<3.8"},
    {version = "^2.0", python = ">=3.8"}
]

oh actually

we could just put our current packages into an optional poetry group

[gritty wind]

We don't have poetry in the image

[fossil veldt]

hm

[gritty wind]

Does pip not support specifiers like that

I guess we could install poetry in the container, it wouldn't make too much of a difference

[fossil veldt]

pip has this format apparently

not sure if it works in command or if we need to use the txt file

[gritty wind]

I don't mind having it moved to a text file really

[tawdry vapor]

Got my hands on a PC again so I'm able to catch up on reviews now :)

[limber widget]

hi

so i normally do lots of math and do the olympiad and stuff like that, but this year i choked on the last round so i dont got much to do, im in g11 right now. I'm looking to work on some app/project that satisfies either being quite technical and maybe using machine-learning and such, or/and is quite helpful and meaningful to the community

dont be afraid to dm me if u have any recommendations for stuff I can work on, or if u would like to work on a project together, and please don't be scared of this being too technical or too challenging. I have years of coding experience, and I have played with machine learning, quantum computing, game development, and a lot of research into theoretical computer science.

[clever wraith]

Hello,

This channel is for discussions regarding our open source projects.

We don't have ML projects and such, and i'll leave the assessment of how technical they are to you.

!contribute if you'd like to contribute, you can refer to this.

[stable mountain]

Contribute to Python Discord's open source projects

Looking to contribute to Open Source Projects for the first time? Want to add a feature or fix a bug on the bots on this server? We have on-going projects that people can contribute to, even if you've never contributed to open source before!

Projects to Contribute to
• Sir Lancebot - our fun, beginner-friendly bot
• Python - our utility & moderation bot
• Site - resources, guides, and more

Where to start

1. Read our contribution guide
2. Chat with us in #dev-contrib if you're ready to jump in or have any questions
3. Open an issue or ask to be assigned to an issue to work on

[fallen patrol]

!pep 621

[stable mountain]

**PEP 621 - Storing project metadata in pyproject.toml**

Link

Status

Final

Created

22-Jun-2020

Type

Standards Track

[outer oasis]

Does Poetry support that part of the PEP yet?

[fallen patrol]

no but you wouldn't have to use poetry

I personally prefer pdm for dependency management like this

/pypi pdm

!pypi pdm

[stable mountain]

pdm v2.4.8

A modern Python package and dependency manager supporting the latest PEP standards

[outer oasis]

Fair
But dropping Poetry is a much bigger change than just adding another group

[clever wraith]

Anyone willing to hope on a call and help me contribute to a GitHub project?

[sharp crag]

Get started with !contributing to one of our repos and ask us here if you need help :)

[clever wraith]

!contributing

[stable mountain]

Contribute to Python Discord's open source projects

Looking to contribute to Open Source Projects for the first time? Want to add a feature or fix a bug on the bots on this server? We have on-going projects that people can contribute to, even if you've never contributed to open source before!

Projects to Contribute to
• Sir Lancebot - our fun, beginner-friendly bot
• Python - our utility & moderation bot
• Site - resources, guides, and more

Where to start

1. Read our contribution guide
2. Chat with us in #dev-contrib if you're ready to jump in or have any questions
3. Open an issue or ask to be assigned to an issue to work on

[fallen patrol]

but you shouldn't use poetry for the venv dependencies.

you don't need snekbox's own dependencies so it would make sense to use something different

you don't need nor want to consider snekbox's dependencies in your lock file

say snekbox depends on attrs transitively through uvicorn (I don't think it does but bear with me). you can't bump attrs to anything that's incompatible with uvicorn even though you want the latest attrs in the available packages for eval

but there's no reason to consider uvicorn when locking dependencies for the nsjail env

so using poetry will hinder you greatly here

it would be better to use something like pdm for this dependency management

pdm can additionally manage and sync requirements files so you wouldn't need pdm in the image at all

make some groups, define python markers if necessary, and export them with the python version as the restriction

[gritty wind]

Or don’t overcomplicate it and don’t use a tool that requires locking lol

We’ll (continue) using pip, probably with what ionite suggested above

[sullen shoal]

@clever wraith tell me

[fossil veldt]

it supports the feature long before that pep, just not in that format

[outer oasis]

Correct
Sorry, my question was "Does Poetry support PEP 621 groups", not "does Poetry support groups"

[fossil veldt]

iirc poetrys argument against it is it would be decreasing their feature set

[outer oasis]

They did
Which is complete bullshit

[fossil veldt]

it is partially true though, the poetry format allows some things that currently pep 621 makes impossible

[outer oasis]

Have they implemented any of PEP 621 yet?
I know they said they would for their 2.0 release, but I don't know what their timeline is for that

[outer oasis]

Accepted
But if they support PEP 621, they don't have to suddenly drop support for their own proprietary format

[fossil veldt]

poetry.lock is already proprietary though

I don't see much point in supporting pep 621

I suppose they could parse it as optional "any of" keys

if only pep 621 also specified a lock file standard...?

[outer oasis]

Nope
It was brought again, then rejected again

[fossil veldt]

looks like poetry rejecting it is enough to get the pep rejected...?

[outer oasis]

It's settled then

Time to ban Poetry

[fossil veldt]

to be fair nobody else really has a working lock file system

[outer oasis]

PDM?

[fossil veldt]

I guess pdm is more flexible with ommiting the features poetry supports?

but it'd be difficult for poetry to suddenly remove these specifications that possibly millions depend on

[outer oasis]

Why would they remove them?

[fossil veldt]

since the proposed standard doesn't allow for poetries additional supported specifications

like local files and vcs

not that poetry is against the pep, they just said they won't use it if it went through

[gritty wind]

The entire concept of dependency managers is incorrect in the first place. We should go back to the good old days of managing dependencies by hand and installing with pip

[vocal prairie]

pip? just copy the files into your project.

[fallen patrol]

pdm lockfiles aren't universal iirc

[fallen patrol]

poetry is too strict for me imo

its good for applications

but if you're writing a library or anything of the sort.... if you choose poetry you're in for a tough time IMO

the lockfile restrictions are what gets me

say you don't want to declare yourself incompatiable with python 4.0
you probably will be incompatiable but why declare it when you dont know
poetry won't let you have any dependencies that declare themselves to be incompatiable with 4.0 which means that if 4.0 were to be released then every dependency of the entire chain would need an update sequentially

this is more pronounced with dependencies that block themselves from 3.11 and 3.12 until they're tested, but 3.12 hasn't even been released yet.

its really quite annoying, and I don't like that lock factor of poetry

on the other hand, for an application, you're responsible for making sure it runs and all of the dependencies play together. For an application it doesn't matter that you can't use python 3.12 until later. But if any of the libraries you're using can't use 3.12, then that'll be a problem and they'll need to make a release, even if they're fully compatiable with 3.12

and poetry pushes people to use this sort of versioning, even when they themselves don't follow the same version scheme

[gritty wind]

Guys, I say this with a heavy heart and complete sincerity: fuck isort

Why does it put relative first part and absolute first part imports into two different groups

Why is that the default style

It's configured to "black" style but black doesn't try to format it like that

[sharp crag]

today is truly a sad day

[outer oasis]

Ur a sad day

[sharp crag]

meanie i cry

[timid sentinel]

black doesn't sort imports at all

[gritty wind]

From the black docs:

Black also formats imports, but in a different way from isort’s defaults which leads to conflicting changes.

All our other projects use pycharm import style, which reduces these conflicts in general

[timid sentinel]

Black format's but doesn't sort them, the isort black compatibility option is to format it like black, but then you can configure how it's sorted on top of that

AFAIK that's how it works anyway

[gritty wind]

Right, which is what I'm saying this is a stupid default style

The comment right above the black one

In what world is a relative import a separate group from an absolute one

I solved it in the end anyway

Just made it an absolute import 😛

[fossil veldt]

why chance it? I wouldn't declare anything compatible if it's not tested and in a CI

allowing you to say you're compatible with 3.13 is not much help when you try to resolve dependencies and it fails because one of your dependencies isn't

[fallen patrol]

because it's not really your place to say you're incompatible with a version that hasn't released yet and you don't know that you aren't.

https://iscinumpy.dev/post/bound-version-constraints/

[fossil veldt]

I don't see what the real problems this causes are

the version bounds allows the resolver to choose an older version of your release if you decide to increase the lower supported python version

[outer oasis]

You have 3.13 in CI??

[fossil veldt]

but if your current release is claiming to be compatible until 4.0, someone installing in 2 years with 3.13 won't know that it's actually incompatible

and instead it fails at runtime

you can't change the version compatibility of the release afterwards

[fossil veldt]

no, that's why I don't claim to be compatible with it

also the lower bound comes up much more in practice, it's easy to just say you're compatible from 3.6, but are all your dependencies?

[outer oasis]

Yes
... I hope
All my tests are passing on 3.6

But that's....
Most of what I've read is in favor of keeping the lower-bounds, just not limiting the upper-bounds, to allow things to update on their own

For libraries*
For binaries almost everyone demands pinning exact versions

[fossil veldt]

ideally python would just use real semantic versioning

then this wouldn't be a problem in the first place

but unfortunately we're in a position now where even security releases can have breaking changes if they decide it's appropriate

[outer oasis]

Is there any language that does this?

Even Rust BTW is doing 1.x.y because of their famous “we’ll never need a v2” shit

[fossil veldt]

say you release

package v1: python >=3.7, <3.12

then 3.12 comes out and you find out it made a breaking change you need to address, but that also makes it incompatible with 3.8 and below.
so you make the change and release

package v2: python >=3.9, <3.13

now people installing on 3.12 will get your v2 package. And 3.7 users can still automatically get your v1 package

but if your v1 package was >=3.7, <4.0 instead, someone who specified ~= with your package will now not automatically update to v2 because it thinks the current version still works on 3.12, which it doesn't

[fossil veldt]

most languages don't make breaking changes to the level python does

[outer oasis]

In what way?

(mostly just ignorant here)

[fossil veldt]

hm?

like C#8 code is compatible with C# 9, and will be compatible with C#10, etc

rust also guarantees compatibility going forward

but code you write for 3.11 might be broken in 3.12

heck code you write for 3.10.6 might be broken in 3.10.7

[outer oasis]

When was the last time this happened?

[outer oasis]

Is this guaranteed?

[fossil veldt]

https://discuss.python.org/t/int-str-conversions-broken-in-latest-python-bugfix-releases/18889

[outer oasis]

I mean, where do they make their breaking changes then?

[fossil veldt]

https://learn.microsoft.com/en-us/dotnet/core/compatibility/library-change-rules

they still have a guideline but it's a lot stricter than pythons

also the language is static enough for the compiler to automatically update your code for future breaking changes

like for example Python's 3.8 breaking change with moving ABCs from collections to collections.abc

C# doesn't even allow these kind of namespace changes, but if they did, the compiler could just automatically update all relevant imports

[outer oasis]

the compiler could just automatically update
Do they have a thing for that?
Is it like dotnet --fix?

[outer oasis]

But... "nothings perfect"... If you decide you don't like what you did, you're just stuck with it forever?

[fossil veldt]

hm, it might be a visual studio thing

https://dotnet.microsoft.com/en-us/platform/upgrade-assistant they have a tool for .NET framework to .NET though

sort of would be like python 2 to 3

[fossil veldt]

also it's really rare that any changes are required at all

usually the IDE auto updates are to let your code use new language features, not that it will be breaking

[outer oasis]

So... when they are?

[outer oasis]

like

[fossil veldt]

like imagine when python added walrus operators

your IDE could have automatically updated your code to use them where appropriate

[outer oasis]

walrus operators are never appropriate

[fossil veldt]

or refactoring try finally to with

[outer oasis]

a wat

[fossil veldt]

like

try:
    f = open("file")
    print(f.read())
finally:
    f.close()

into

with open("file") as f:
    print(f.read())

in c# IDEs can automatically make this feature upgrade

python would be too dynamic to guarantee these do the same thing, so it's not possible

[outer oasis]

Well that's just an argument for static over interpreted not breaking changes

[timid sentinel]

I don't really see what semantic versioning would help with there tbh

It's such a large language, a breaking change for someone could be completely irrelevant for 99.9% of people

[fossil veldt]

it won't really, I guess what I meant was there ideally the minor versions shouldn't be breaking backwards compatibility this frequently

[outer oasis]

We'd just make sure our upper bounds kept us below 425.44.17post2-breaking-change-of-the-day-45 until we're good and ready to update

ezpz

[fossil veldt]

what do you mean by large?

[fossil veldt]

well, ideal in terms of package compatibility, there are certainly benefits to the language being able to make changes quickly and move forward

I'd estimate that each new minor python release breaks 40% of packages by transient dependencies

[outer oasis]

.... Where'd you get that number from?

[timid sentinel]

semver doesn't really differentiate between "small technically breaking change in barely used function" and "hey guys we made print a function not a statement", and with such a large interface and lots of changes, there's likely to be a lot of breaking changes for 0.1% of people that are noise for the other 99.9%.

Maybe that's more of an argument for a fixed release schedule though

[outer oasis]

How would that help?

Python has one, doesn't it?

A new release every October

[timid sentinel]

yeah, my thinking is that if releases were made more often then they'd likely end up being breaking changes more often and you'd have to check every time if it affected you which it probably usually wouldn't. By grouping it into one release a year it's more manageable to check what affects you.

[timid sentinel]

Do you think that the changes shouldn't be made at all? Or that they should be 'released' even less often?

[fossil veldt]

maybe to a lesser extent

it won't help that much though, most of the reason so many packages break in minor releases is due to C-API changes

and many major packages with a huge amount of transient dependencies rely on the C-API

if anything we need more guarantees and public APIs on the C side, so people can write C extensions without messing with potentially breaking internals

python integers (PyLongObject) will have a new C structure in 3.12 it seems, and will probably break every existing non API usage of them (which, there wasn't an API before)

[outer oasis]

They restructured one of the builtin types?

[fossil veldt]

nobody is really at fault, it's just the position we're in

[outer oasis]

That sounds like a huge change

[fossil veldt]

oh they do that a lot

particularly dicts and strs went through a bunch of changes

code/frame objects change almost every release

[outer oasis]

[fossil veldt]

usually for optimization

[outer oasis]

So... Now I'm thinking about my packages
So... If I have https://github.com/darbiadev/darbiadev-ups/blob/main/pyproject.toml#L10-L14
In your opinion, what should I update it to?

[stable mountain]

pyproject.toml lines 10 to 14

requires-python = ">=3.7"
dependencies = [
    "requests>=2.26.0",
    "typing-extensions>=4.3.0; python_version < '3.8'",
]```

[outer oasis]

Half of me also wants to just drop support for <3.10 so I don't have to worry about typing and shit

[fossil veldt]

3.8 is not that hard to support

3.7 makes typing pretty difficult and no walrus

[gritty wind]

I agree that you should lock that upper-limit, but I don't have opinions on the rest

[outer oasis]

So... If my only dependency is requests, you still think I should set Python to <3.12?

Should I set an upper limit for requests too?

[gritty wind]

Scratch that, I have a second opinion: Using requests smh

I've seen libraries get bitten by that before, you should ideally have an upper limit for everything

[fossil veldt]

you can ~= to use compatible versions

[gritty wind]

If requests decides to release a v3 you're SOL retroactievly

[fossil veldt]

so if requests updates to 2.27.0 it won't automatically update

it's not that much work since dependabot just PRs

[outer oasis]

Yeah

[fossil veldt]

but at least you can see the CI change from the PR and see if its fine

how ~= works is a bit weird though

requests~=2.26.0 will work as I described, if you did requests~=2.26 it would instead auto update on the minor version

[gritty wind]

If you’re looking for something a little easier to understand, star works pretty well in most cases

It has its quirks too

[outer oasis]

Star?

That's a thing?

[fossil veldt]

you mean ^=?

[gritty wind]

I’m referring to 2.26.*

[fossil veldt]

ah

[outer oasis]

... thats a thing?

[gritty wind]

It’s functionally identical to the above ~2.26.0

[fossil veldt]

lolwut apparently === is also a thing

[gritty wind]

But can’t reproduce something like

[fossil veldt]

js in python

[gritty wind]

~2.26.3

[fossil veldt]

https://peps.python.org/pep-0440/#arbitrary-equality

[gritty wind]

But that’s a pretty rare requirment

[fossil veldt]

An example would be ===foobar which would match a version of foobar.

This operator may also be used to explicitly require an unpatched version of a project such as ===1.0 which would not match for a version 1.0+downstream1.

curious

[outer oasis]

... there really is a thing for everything

[fallen patrol]

so what's the staff gonna do when this server gets closer to 500k members (||this is satire||)

[gritty wind]

Any idea why this is reporting a coverage-miss https://coveralls.io/builds/57989942/source?filename=snekbox%2Fapi%2Fresources%2Feval.py#L136

It should be covered by tests.test_integration.IntegrationTests.test_eval_version

I can't get a proper debugger in, but I've added a print with that if statement, and I definitely see Trues and Falses, so it should have covered the line in both cases

Do the integration tests just not count towards coverage or something

[cold island]

link to the test?

I'm guessing it's https://github.com/python-discord/snekbox/blob/multi-version/tests/test_integration.py#L70

[stable mountain]

tests/test_integration.py line 70

def test_eval_version(self):```

[gritty wind]

Yeah that's the one

If it's any help, I'm getting the same thing locally

The coverage message is a bit less cryptic than the coveralls one:

line 135 didn't jump to line 136, because the condition on line 135 was never true

[cold island]

what's up with the extra () in the snekbox_request call

not important to the question

[gritty wind]

I didn't notice that

I was copying from the function above which is structured differently

I'm really starting to hate black

Why is snekbox randomly using it

[cold island]

I don't have a concrete idea for the coverage issues, but I noticed that the specific version tested is dynamic in the test, and that might affect the coverage in some cases. I would make sure to test both a non-main and a main version

for example if you sent the main version and it's the first one in ALL_VERSIONS, then 137 in the tested code is never False

[gritty wind]

That's not quite right

If you pass main version as the version, it'll still be true, since it'll parse the version from the body regardless

The reason I bother with looking for a non-main version is to make sure everything checks out in that department, and that multi-version support works

[cold island]

yes I'm saying it'll be true

[gritty wind]

Ah I see what you're saying

But it'd still be false everywhere else in the codebase

Since no other request actually sets body

The if condition doesn't actually care about this being main or non-main

It's hard to write the tests to ensure both cases get tested without coupling the tests to the current versions config

[cold island]

right, but in terms of the logical flow of the code, if it's the first version then there won't be an iteration where it's false

[cold island]

you could set the value for ALL_VERSIONS for the sake of the test

[gritty wind]

I don't follow this

[gritty wind]

That would error out if you pass an invalid version, since this is actually a full test where it goes through the entire process including eval

[cold island]

If you hit this on the first iteration, then you don't have a case where you skip line 138

[gritty wind]

Ohhhhh you mean that

Yeah I guess that's true, but I'm not particularly worried about that

I guess we could loop in reverse in the test

Actually wait

[cold island]

it'll always be an issue if you only have one version

[gritty wind]

There is indeed never a case where you skip line 138

That's intentional design

[cold island]

wdym

you sent a specific version

[gritty wind]

If version is set in body, it's guaranteed to be a valid version by the validator, hence that line will always produce a hit

[cold island]

for one of the versions in ALL_VERSIONS

not all of them

so for one of the iterations, it will be skipped, and then hit for another

[gritty wind]

I don't think that's measured in coverage

Just whether you branched into true/false

Ah I see what you're saying

[cold island]

right, but I'm describing a case where there's no false

[gritty wind]

I'm not particularly concerned with covering that case for now

It won't have a clean solution unless I go and start mocking a bunch of stuff

One could argue that means a clean solution does not exist at all 😛

[cold island]

is there a coverage requirement on snekbox?

[gritty wind]

Nah, but ideally we'd keep that quite high for something that's centered around security

[cold island]

right

[gritty wind]

There was a sharp drop a while back, I'm guessing maybe file-io?

[cold island]

do you like, have the .coverage file somewhere?

[gritty wind]

Sure, I can send it here if you like

[cold island]

sure

[cold island]

hmm that's a lot gibberish lol

I just wanted to see the table

[gritty wind]

Are you inspecting it by hand

I could send you the output of coverage xml or html

Probably in DM since it needs to be zipped up

[cold island]

yeah whatever is legible

ah ok it's much clearer now

ALL_VERSIONS is empty

https://github.com/python-discord/snekbox/blob/multi-version/scripts/python_version.py#L26

[stable mountain]

scripts/python_version.py line 26

ALL_VERSIONS: list[Version] = []```

[cold island]

Where is it filled?

[gritty wind]

Under that it checks if it's empty and fills it

I realize that's kinda confusing

It's an artifact from a refactoring

Let me re-refactor

[cold island]

Can you make sure it's filled in the test?

as a sanity check

[gritty wind]

You mean to debug this issue, or just a unittest for this in general?

I've checked the former

[cold island]

the former yeah

[gritty wind]

So the code in the python_versions should be doing the latter actually, there's an assertion there which ensure a primary version exists

I've checked this test by hand, and it's definitly finidng a version, and definitly sending it in the body

one sec

print(body) in eval:

{'input': 'import sys; print(sys.version)', 'version': 'CPython 3.7', 'args': ['-c', 'import sys; print(sys.version)']}

This is based on the update I had just pushed adding 3.7

[cold island]

what is the value of ALL_VERSIONS there?

[gritty wind]

[Version(image_tag='3.7-slim-buster', version_name='3.7', display_name='CPython 3.7', is_main=False), Version(image_tag='3.9-slim-buster', version_name='3.9', display_name='CPython 3.9', is_main=False), Version(image_tag='3.11-slim-buster', version_name='3.11', display_name='CPython 3.11', is_main=True)]

[cold island]

hmm... I don't see a .coveragerc file, so I don't see why coverage would skip it

[gritty wind]

Coverage is configured in the pyproject for snekbox iirc, but i don't think it's set to be skipped either way

I added a few prints inside the block, and I can see output for each step

The first if statement, the loop, the inner if

I don't think it's a code issue

[cold island]

right... omit = ["snekbox/config_pb2.py"]

[cold island]

an issue with coverage?

[gritty wind]

It's an autogenerated file, we don't test it

[gritty wind]

Hmm, maybe, or I think what might be happening

[cold island]

yeah yeah, I'm just saying there's nothing to omit it

[gritty wind]

Is since this is going through a request

Which I think is forking the process

It's not actually evaluating within the context of coverage

Or rather, the gunicorn where that coverage would happen is going through a forked process

[cold island]

makes sense, but then is there any code which calls the function directly? since parts of it are covered

[gritty wind]

No by the looks of it

Hm

I did a search for on_post in the entire project and the only hit was within the file itself

But I know that these routes are getting hit by something reagrdless

My new file with no tests has 92% coverage

Actually

There's a simulate_post within the testing framework which is probably what's doing a lot of this

I might be able to resolve the issue by keeping this integration test and just adding a unittest which does mock the eval

[gritty wind]

Ok great, I fixed this issue, and the one you had pointed out

Turns out I didn't have to do any mocking because the API tests already mock out eval

[hoary haven]

heya how is .bm comeback coming along? @sharp crag

[cursive relic]

Are we getting the text command back, the .bm?

[clever wraith]

Yes

Robin is currently working on it

[cursive relic]

Ah, that's great 👍

[sharp crag]

Looks like Chris left a review - I'll get to those today 😁

[rapid swallow]

https://github.com/python-discord/bot/pull/2467

Cool PR ngl

Would look even cooler with one more approval/review

[slim venture]

how does the scheduler work

[cold island]

https://github.com/python-discord/bot-core/blob/main/pydis_core/utils/scheduling.py

[slim venture]

does the scheduler work after restarting?

[cold island]

no

[cold island]

Hey @sharp crag @brisk brook can one of you maybe pick up https://github.com/python-discord/site/pull/702 ?

[clever wraith]

@last patio Is there a reason we removed the note in site#912 that that the channel is not related to Discord patners ?

[dusky shore]

GitHub

[site] #912 Update mention of the partners channel

[cold island]

Ah we just removed it along with the comment that we're partnered. Didn't seem important

We could expand the description a bit maybe

[clever wraith]

Yeah, it's not super important, i just thought it removes some ambiguity

[outer oasis]

https://github.com/python-discord/bot/blob/main/bot/exts/moderation/modlog.py#L122

[stable mountain]

bot/exts/moderation/modlog.py line 122

embed.timestamp = timestamp_override or datetime.utcnow()```

[outer oasis]

Why is the mod log in UTC?

[vocal prairie]

iirc the time it takes is timezone aware, and when in the actual timestamp field, discord automatically localizes it

[outer oasis]

timestamp – The timestamp of the embed content. This is an aware datetime. If a naive datetime is passed, it is converted to an aware datetime with the local timezone.
It's giving me "tomorrow"

[vocal prairie]

wait uh. was utcnow the thing with the nightmare that it's actually a naive stamp or something?

i don't fully understand datetime's design decisions, so take what i said earlier with a pinch of salt

all i know is that it does indeed work

[tawdry vapor]

Looks like a bug. It was probably missed when upgrading discord.py to 2.0, which changed the way it dealt with datetimes.

It's not manifested in production because the prod environment uses UTC as the local timezone

[cold ibex]

ello

[stable mountain]

:incoming_envelope: :ok_hand: applied timeout to @cold ibex until <t:1679512529:f> (10 minutes) (reason: duplicates rule: sent 4 duplicated messages in 10s).

The <@&831776746206265384> have been alerted for review.

[dull swan]

I have no idea how to report this, but is anyone tracking on the modmail ticketing system just... not working sometimes?

This is pretty consistent; if you have any logs of the modmail interactions, I'd be interested to see if these requests are making it to you guys at all.

[gritty wind]

We don’t get any insightful logs, the issue just generally means you didn’t react to the bot in the allocated time

I see you clicked on the check mark, so I’m not sure what’s up with that, modmail isn’t the best software

Are you clicking on the reaction as soon as it comes up?

[cold island]

@clever wraith let's move here, talking on github is a bit annoying when we're both online 😛

[clever wraith]

Ahahahaha, sure

[cold island]

First of all, do you agree using null for "not edited" is an issue?

[clever wraith]

Yes, it's confusing when it comes to permanent infractions.
Can't we have a default for that?

There are approaches to fixing this of course, but not sure of how clean they are. Which is why I went for separate columns for each editable value

[cold island]

It's not just confusing, it breaks your code. Even if you only used that table to display information, you wouldn't know what information to display in that case

[cold island]

If it's a timestamp field in the DB then you're limited to what a timestamp can be

[clever wraith]

Can you elaborate on How will it break the code? I can't type alot since I'm on mobile.
But I see our infractions table as the source of truth here for infractions.

[cold island]

They both need to be sources of truth. The infractions table is the aggregated state of all the actions, but the actions are an audit log of how the infraction transformed over time.

[clever wraith]

I think i see

So what you mean here is

1. I make a perm infraction, value for expiration is null
2. I make it non-permanent, so it has a value now in both tables
3. I make it permanent again, eg give it a null value. This will mark it as null in the actions table so we can't know what happened anymore

[cold island]

yes

So we would either need to change the representation of permanent like you said (I'm not sure what), or find another solution, which is why I brought up the JSON idea in the OP

As for

I was mostly thinking of a trigger at the db level upon insertion / update to the infraction table.

I was thinking the opposite. If you make an edit it seems natural to POST a new action and for it to trigger a change in the infractions table

[clever wraith]

What would the json hold? Keys like reason and expiration?
I'm not a big fan of having json values in a db if they can be avoided to be honest. As a column is more of a clearer structure than a json.

Changing the definition of permanent seems like an easier solution, we'd just need to define it

[cold island]

yeah I'm not a fan of it either, but if we were to go for it it would be an "edits" field that holds any new values sent. "reason" if the reason was edited, and "expires_at" if the expiration was edited

[clever wraith]

Would this happen as a trigger as well on the db level? At site's level, or at client(bot)'s level with 2 requests

[cold island]

"expires_at" is currently a DateTimeField, so if we're changing the value of permanent we're limited to what this type of field can hold.

[clever wraith]

Having 2 queries at site level seems like a good approach

[cold island]

yeah the bot sending one request and the site processing it seems ok

I think this is what you meant lol

[clever wraith]

Ahahahaha yes

Ok, you and I agree on that part.
Now for the expiration

Instead of changing that, can't we have a value for unedited fields? Like ''unedited'' or whatever?

[cold island]

How would that work? a field has a defined type, and values need to adhere to it

[clever wraith]

It would work by changing the datetime to a string literal then casting it, but that's a bad idea, so forget I even mentioned it ahaha

I'll try to think of something.
My brain is foggy now as I'm fasting, but if you have any ideas please shoot them so that we'll lay everything on the table.

Upon rescheduling infractions when stating the bot, it'll only fetch the ones whose expires_at isn't null, right?

[cold island]

yeah

hmm

[clever wraith]

I was thinking of changing values of permanent infractions to a datetime in the past, but that's not clean at all I suppose

[cold island]

yeah active non-permanent

[clever wraith]

So they query used to fetch those will exclude the ones with that value.
But I'm not a fan of this either

[cold island]

yeah saying that a permanent infraction has a null expiration seems logically sound

[clever wraith]

True true

I'm seeing the json solution approaching ahahah

[cold island]

haha yeah, I mentioned it with a heavy heart

[clever wraith]

Can't we have type unions in db?

[cold island]

That's beyond what I know about pg but I can check

[clever wraith]

I don't seem to find an out of the box answer to this, so I'm guessing even if it's doable via some shenanigans, it's probably not the best idea

[cold island]

yeah me neither. We could add another field that if not null, gives a meaning that's different from a datetime. But it's very meh

[clever wraith]

This is growing in size in my head ahahaha

I'll try to recap everything we discussed here on the issue, that way people can add their input if they have any

[cold island]

We can have this conversation in a few hours/another day if you want

[outer oasis]

Have you tried this wonderful new idea called eating?

[clever wraith]

It's ramadan here

[outer oasis]

Ah

[clever wraith]

Aha!