makes sense, will ask on the issue
#dev-contrib
timid sentinel
dry folio
hey @patent pivot, the minor pr is here https://github.com/python-discord/bot/pull/1719
patent pivot
rock solid
dry folio
@vale ibex left some comments on your review :D
fallen patrol
@green oriole i'm pretty sure making the emojis not part of the hyperlink would fix it
vale ibex
🤔
vale ibex
sure
viscid coral
How did you do the Github webhook update? I know you told me once about it, and I did everything there but it says 404
I tried the webhook url alone and the full_link/git
vale ibex
GitHub is giving a 404 error?
viscid coral
vale ibex
did you set the content type to application/json?
viscid coral
Yeah
vale ibex
hm that's odd
and the webook is the full https://discord.com/api/webhooks/<id>/<secret>
viscid coral
It's webhooks for me'
vale ibex
yea
viscid coral
Oh yeah
vale ibex
I think it's /github
viscid coral
Now it shows 400 instead of 404
vale ibex
try with /github on the end?
vale ibex
Nice
clever wraith
and the webook is the full https://discord.com/api/webhooks/<id>/<secret>
You need /GitHub at the ebd
vale ibex
Yea, scroll down
dim pelican
Hey all, just getting started with contributing to projects. It seems like bot, site and lancebot seem to be the largest / most active repos. Any advice for starting to contribute / review PRs / opening a PR for an Issue?
brisk brook
You can read our Contributing Guidelines: https://pythondiscord.com/pages/guides/pydis-guides/contributing/
If there's any issue you get while setting up the projects, opening pull requests, or opening issues just ask.
dim pelican
You can read our Contributing Guidelines: https://pythondiscord.com/pages/guides...
Thanks!
clever wraith
Yea, scroll down
Lol woops
green oriole
`!ext reload *`
That's a thing, yes
fallen patrol
tbh i don't have the entire source
green oriole
!source ext r
stable mountainBOT
green oriole
stable mountainBOT
bot/exts/utils/extensions.py line 19
UNLOAD_BLACKLIST = {f"{exts.__name__}.utils.extensions", f"{exts.__name__}.moderation.modlog"}```Hmm
fallen patrol
ah
green oriole
!ext r ext
fallen patrol
one sec
stable mountainBOT
:x: Could not find the extension ext.
green oriole
!ext r extensions
stable mountainBOT
:ok_hand: Extension successfully reloaded: bot.exts.utils.extensions.
fallen patrol
you can reload it, but not unload
green oriole
Ah, it is reloading the utils
fallen patrol
its an okay blacklist
green oriole
Or is it not
fallen patrol
because if you try to reload when it has a problem it won't actually error
green oriole
Yeah, seems like this bl isn't applied to reloading
fallen patrol
as it doesn't need to be tbh
green oriole
But it shouldn't error when being unloaded
green oriole
You just remove the handlers, that's it
celest charm
Hey people, could we perhaps add a guideline on formatting URLs?
I think that whenever possible, URLs should not be formatted with str.format or f-strings, and instead use session.get(BASE_URL, params={"q": "..."}).
- it's not prone to URL injection
- you don't need to create an additional sanitized string
- you don't need to make sure you use it and not the original one
- you don't need to check the docs/implementation of
quote_plusto make sure it indeed does not just change spaces to+s)
celest charm
can you link where it is?
green oriole
celest charm
I don't think it's really a style issue, it's more of a practice issue
green oriole
I was looking for the link :D
Hmmmm
I think this page is what makes the most sense
celest charm
maybe we need a separate document for practices to avoid?
vale ibex
We could have it as a tag
or if you want something longer form, you could have it as a resource on site
celest charm
I think we should wait until we have a few practices we have to document, and then collect them
but I've seen this URL formatting issue in a few places in the bot, we even had a case where the query wasn't properly escaped (which allowed URL injection)
celest charm
Hey people, could we perhaps add a guideline on formatting URLs?
I think that wh...
case in point: the !pypi command used to format a string like that, which led to unexpected errors when you added an & to the package name
brisk brook
dry folio
!user
stable mountainBOT
You are not allowed to use that command here. Please use the #bot-commands channel instead.
dim pelican
I think I have sir-lancebot up and running on my test server, is there anything else I should do prior to contributing?
gritty wind
There isn’t really any thing, and people are happy to help you along the way if you run into any roadblocks
Have a look at the issues section on GitHub, and if you see something you’d like to try, you can drop a comment on the issue. Alternatively you can look at a few PRs, possibly review them, to see what others are doing
molten perch
Hey! Is sir-lancebot#743 still valid? I mean I made my solution a long time ago, however it involved changes in the issue command as well, so I waited because the state of the command was dubious. (sir-lancebot#714 sir-lancebot#788)
dusky shoreBOT
vale ibex
I have closed the latter two issues, I think the first is still applicable
Since it's merely improving the embed we have now, rather than changing the issue command
I'd wait until sir-lancebot#778 is merged though
dusky shoreBOT
molten perch
Yeah, actually that's what I'm doing. (Even left my first review on that 😄 )
molten perch
Yeah, it was a bit messy at first though 😄
dim pelican
Now that there is a stackoverflow searcher, how about a Real Python equivalent?
green oriole
That'd be sick! Real Python is so good
dim pelican
I don't think they have an API defined yet, so I just sent them an email through their site. Hopefully they respond back and are open to the idea!
stable mountainBOT
vale ibex
Ah cool, it's pulls from their rss feed
stable mountainBOT
gritty wind
If we’re going to be scraping anyways, we could skip the dep
tawdry vapor
I want to start using arrow more, but a lot of functions are only annotated to accept datetime.datetime. While arrow.Arrow's interface is a superset of datetime.datetime's, the former doesn't actually subclass the latter. Should I create a type alias that unions them? Kind of annoying to have to import that :(
fallen patrol
I want to start using arrow more, but a lot of functions are only annotated to a...
wdym?
patent pivot
hmmm
yeah that's an interesting one
I'd go for the union type and take the hit on the imports
tawdry vapor
Thanks Joe. I'm still open to any other ideas.
fallen patrol
While arrow.Arrow's interface is a superset of datetime.datetime's, the former doesn't actually subclass the latter.
if its a superset of datetime.datetime wouldn't it be inheriting from datetime.datetime
tawdry vapor
No. It manually implements the same interface without using inheritance.
fallen patrol
ah
tawdry vapor
Maybe not totally manually, I don't know how exactly they do it. But ultimately it's not inherited.
tawdry vapor
Convenience.
brazen charm
not sure if you can do much more than an union or something else that'd have to be imported if it should be understood by checkers
tawdry vapor
It has a nicer API. For example, to convert an ISO 8601 string just arrow.get(). Shift a time with .shift() instead of creating a timedelta. Also utcnow actually returns an aware datetime
fallen patrol
ah
oh
uh
If you're making all of these changes, I would keep in mind one thing about discord py v2
every time provided by discord.py was changed to be a timezone aware datetime
tawdry vapor
I wasn't aware, but I am happy they are changing that.
Actually, all the stuff I've been working on will make the transition easier.
fallen patrol
yeah, would just keep that in mind when changing these times so its just slightly easier lol
I'm just starting a bot on dpy 2 rn
tawdry vapor
All the time utils are having their inputs normalised to UTC aware datetimes.
fallen patrol
lol
I recommended joe use dpy 2 on @radiant merlin since its from scratch
I'm taking some credit for that :P
tawdry vapor
I wish arrow had Duration from pendulum, cause it can provide totals in any units.
In fact pendulum does subclass datetime types, unlike arrow. However, the lib isn't as well maintained so I'm not inclined to switch to it.
patent pivot
~~I recommended joe use dpy 2 on <@!861747395632037890> since its from scratch~~
you said BK but we figured we shouldn't do that on important infra lol
static canyon
I believe KA is 2.0, but BK is 1.7
fallen patrol
ah
static canyon
https://github.com/python-discord/king-arthur is the source
fallen patrol
does BK actually do anything yet?
static canyon
And yeah, KA is 2.0
fallen patrol
https://github.com/python-discord/king-arthur is the source
oh lmfao
just saw this taskipy task
format = "black arthur"
I'm sorry, its just a funny cl
patent pivot
so <@!861747395632037890> is back on 1.7?
KA is 2.0 since it's non-crit, BK is 1.7
fallen patrol
yeah
patent pivot
does BK actually do anything yet?
few things in the works
patent pivot
nothing that will affect the layman
viscid coral
@surreal veldt did you open two issues about the same thing?
viscid coral
viscid coral
Oh I see
static canyon
Potentially when they edited it
viscid coral
I just wanted to know because I am not good with github
static canyon
One was 9 mins ago, the other 8 and edited
So normal: no, but also not a bug (user just accidentally opened two issues)
viscid coral
Yeah it is, thanks
green oriole
If you look at the issue ID in the embed, 1722 and 1723, you can see they are actually two different issues
static canyon
(And also have different titles I guess)
tawdry vapor
Would it be possible to replace the allowed_strings converter with typing.Literal, or would support have to be added on discord.py's end?
Related: it bothers me that type checking is confused when custom converters are used. Is it possible to address that? Maybe have the converters inherit from the type they return, assuming they only return one type. Or some metaprogramming trickery (not sure if type checkers would be sophisticated enough for that).
On second thought, the subclassing idea seems problematic because convert is an instance method, meaning discord.py has to instantiate the converter class first.
Hold on, this sounds perfect https://discordpy.readthedocs.io/en/latest/ext/commands/commands.html#inline-advanced-converters
dim pelican
Maybe a silly question, but if there was a pull request to sir-lancebot that included a new package, how would you go about adding that package to sir-lancebot?
brazen charm
poetry add "package~ver" in most cases
brazen charm
That would be the poetry file now
pyproject is the poetry file no?
green oriole
Yup haha
timid sentinel
relocking seems to break sir-lancebot at the moment
I get AttributeError: module 'aioredis' has no attribute 'ConnectionsPool' in fakeredis, seemingly because aioredis upgraded to 2.0 (• Updating aioredis (1.3.1 -> 2.0.0)) which fakeredis doesn't seem to support. Not sure if that's because it doesn't define it's requirements properly or something...
dim pelican
And then add the poetry.lock file in the commit?
green oriole
I get `AttributeError: module 'aioredis' has no attribute 'ConnectionsPool'` in ...
Do we do a ^1.3 or ~1.3 pin?
timid sentinel
we don't specify aioredis in our requirements
timid sentinel
yeah
green oriole
I believe explicitly pinning to ~1.3 should fix it though
timid sentinel
yeah
brazen charm
fakeredis does specify aioredis<2 while async-rediscache does aioredis>=1 so I guess it's just because of how poetry resolved the fakeredis aioredis extra
timid sentinel
oh huh, is that a poetry bug then? Or more of a limitation maybe?
brazen charm
I think it is because async-rediscache doesn't specify fakeredis[aioredis] for the extra, no idea how they all interact though
short snow
Andd, I will get back to my PRs, I had been on break for 2 weeks so wasn't active
timid sentinel
I believe explicitly pinning to `~1.3` should fix it though
That seems to fix it, would tht be worth a PR then, could bundle it in with https://github.com/python-discord/sir-lancebot/issues/794 if that's appproved. Unless @dim pelican working on a PR requiring a new package so would need it in that anyway?
timid sentinel
And then add the poetry.lock file in the commit?
Also yeah you would commit the poetry.lock if you're adding/updating a module
brazen charm
@hardy gorge is that something that should be fixed for async-rediscache?
green oriole
That seems to fix it, would tht be worth a PR then, could bundle it in with http...
if that's appproved.
Wdym, it is approved
But yeah, whichever PR changes deps first
brazen charm
the other pr can just relock when merging depending on what's merged to main first
dim pelican
@timid sentinel I don't have a PR ready to go yet, but I have some ideas for two in the next few weeks! Thanks for the feedback and advice, I hope I can start to contribute meaningful things soon
hardy gorge
<@!336843820513755157> is that something that should be fixed for async-rediscac...
To be sure, because I'm on vacation with no access to my own PC, you say it should be the following?
install_requires=[
"aioredis>=1, aioredis<2"
],
Adding discord tags to the stackoverflow searcher sir-lancebot#797
Creating a new command to do something similar but with Real Python (no issue opened yet)
dusky shoreBOT
brazen charm
To be sure, because I'm on vacation with no access to my own PC, you say it shou...
yeah, I thought it was just a fakeredis issue from the error above but the 2.0.0 aioredis ver had breaking changes that don't work with how rediscache does things
but I think fakeredis with the aioredis extra should also be specified too if I understood it correctly as it is used with fakeredis, to make it apply its own constraints in case something changes
tawdry vapor
Would it be possible to replace the `allowed_strings` converter with `typing.Lit...
So.... Any ideas?
brazen charm
I think support for Literal as a converter is in some (probably yet unreleased) version of discord.py
green oriole
This is an interesting idea but I have no idea if that's feasible. If it is it sounds like a good change.
brazen charm
I recall seeing it somewhere but right now I can only find it in the source code and translated docs
For doing proper type hinting with converters I'm not sure if there's much we can do, maybe patching discord.py's handling to get the converter or other data out of typing.Annotated but there's probably more to that
tawdry vapor
For doing proper type hinting with converters I'm not sure if there's much we ca...
For this I believe I stumbled upon the solution myself at the end with that link. Just subclass the type and add a convert classmethod, which will be fine as long as convert isn't overriding something on the base class.
brazen charm
For this I believe I stumbled upon the solution myself at the end with that link...
It's better than the current way, but it'll still be wrong when the command is called directly and can't really express things like Optional
I think the Annotated typehint would be ideal but I don't see anything on discord.py's github so I have no idea if it isn't there yet because of the support for older versions, some other reasons or just hasn't been considered
tawdry vapor
I don't see how it will be wrong, and what optional has to do with it.
short snow
Rapptz/discord.py#6619
dusky shoreBOT
short snow
Are you talking about this one?
brazen charm
I don't see how it will be wrong, and what optional has to do with it.
For example
class Converter(int):
...
def command(a: Converter): ...
def call_command():
command(123)
Oh right.
Can't really solve that
Your example is poor though cause you're actually using the right type
brazen charm
It's the right type, but wouldn't pass through a checker because it's not Converter or its subclass
for the Optional, you can't subclass NoneType or anything to provide that information
tawdry vapor
It's the right type, but wouldn't pass through a checker because it's not Conver...
That has to do with contravariance? I wasn't even thinking about that. I can live with it though; the commands are rarely called directly and either way the type is incorrect.
tawdry vapor
for the Optional, you can't subclass NoneType or anything to provide that inform...
You must be talking about the return type then? I did acknowledge that this only works if it returns one type.
brazen charm
Ah didn't notice that, multiple inheritance would probably work for multiple types but the subclassing as a whole is more limited than normal typing through the types that provides. Although still better than the current system
patent pivot
lol i didn'y
sinful knot
#community-meta message
which rules should I add beside __?
vocal wolf
https://canary.discord.com/channels/267624335836053506/429409067623251969/873286...
I think there's a discord.py utility to escape markdown, let me try to find it
fallen patrol
so
fallen patrol
I think there's a discord.py utility to escape markdown, let me try to find it
there is, doesn't work on all things iirc
green oriole
It shouldn't work for everything
fallen patrol
green oriole
We still want codeblock, bold and italic
fallen patrol
but only * bold and italic
fallen patrol
not _italic_
vocal wolf
and you already got it, nvm lol
fallen patrol
yea lol
sinful knot
so I shouldn't use it?
sinful knot
then which rule should I add beside __
celest charm
Why aren't we using mypy or other linter for our projects in the CI?
Type hints are great, but they have negative values when they are wrong. For example:
@staticmethod
def find_vowels(displayname: str) -> str:
"""
Finds vowels in the user's display name.
If the Discord name contains a vowel and the letter y, it will match one or more of these patterns.
Only the most recently matched pattern will apply the changes.
"""
expressions = [
("a.+y", "patchy"),
("e.+y", "ears"),
("i.+y", "ditsy"),
("o.+y", "oofy"),
("u.+y", "uffy"),
]
for exp, vowel_sub in expressions:
new_name = re.sub(exp, vowel_sub, displayname)
if new_name != displayname:
return new_name
The linter enforces both the type hint existing and the docstring existing, but it doesn't force either to say that it can return None
i.e. that the type hint should've been Optional[str]
tawdry vapor
It was never seriously considered, but I believe the general feeling used to be that it was deemed not worth the trouble to try to conform to such strict type checking.
I don't mean that your example shouldn't be fixed.
celest charm
I guess it also doesn't help that discord.py uses annotation-based converters which just break type checkers
tawdry vapor
Yeah, but we were just discussing a possible workaround for that. It wouldn't cover all cases, but would help with the majority of uses.
celest charm
Maybe it shouldn't fail the CI, but it would be nice if it reported some sort of warning for new code that doesn't pass mypy/pyright
tawdry vapor
I'm open to the idea.
celest charm
(full disclosure: I'm a total noob and I'm not vouching to set up and/or monitor this infrastructure)
celest charm
noob with CI
tawdry vapor
Okay, that's not a problem. That part is easy.
celest charm
we can try mypy and pyright, I think
brazen charm
I'll take a look at Annotated and if it can be made to work properly on bot, which would make the converters play nicely with any checker that understands it
celest charm
If we run it on the whole code, mypy will have a heart attack, so I was suggesting on only reporting issues in new code
I want to make a PR that fixes some incorrect typehints to seasonalbot today, but I'm worried about creating merge conflicts with PRs, is that fine?
tawdry vapor
If we run it on the whole code, mypy will have a heart attack, so I was suggesti...
How would that work? Would it just run it on a single file? What would happen if something imported has a missing or wrong type?
celest charm
How would that work? Would it just run it on a single file? What would happen if...
We could collect all errors, but only report those inside the changes (by parsing the output, since it has line numbers)
(but maybe it's a really stupid idea)
tawdry vapor
I want to make a PR that fixes some incorrect typehints to seasonalbot today, bu...
Waiting for all PRs to be merged is a lost cause. There are almost always new ones opened before all of the previous ones close. Just make the PR and people will deal with the conflicts. That's the reality sometimes.
celest charm
👍
Do devops people/core developers/anyone else get a ping when the bot fails with an exception? @patent pivot
patent pivot
Do devops people/core developers/anyone else get a ping when the bot fails with ...
no, but it'll end up in sentry
if it's new then there is a webhook to discord and an email
celest charm
no, but it'll end up in sentry
Could you please show the exception from this?
<#mod-spam message>
wdym by "it's new"?
patent pivot
08/06/21 22:33:06 - bot.exts.evergreen.error_handler ERROR: Unhandled command error: list indices must be integers or slices, not str
Traceback (most recent call last):
File "/usr/local/lib/python3.9/site-packages/discord/ext/commands/core.py", line 85, in wrapped
ret = await coro(*args, **kwargs)
File "/bot/bot/exts/evergreen/githubinfo.py", line 53, in github_user_info
org_data = await self.fetch_data(user_data["organizations_url"])
TypeError: list indices must be integers or slices, not str
yup
patent pivot
if the exception has occurred before
as in has the same exception fired with the same stacktrace
celest charm
ah, so this fired before?
patent pivot
nope, this is new
celest charm
alright, thank you joe master
I'm on a quest to review the entire seasonal bot and find all URL injections lol
patent pivot
lol
fallen patrol
I'm on a quest to review the entire seasonal bot and find all URL injections lol
what's seasonalbot?
seasonalbot#1
dusky shoreBOT
fallen patrol
ah, lancebot
fallen patrol
I'm so old I remember it being Sir Lancebot <:lemon_pensive:754441880246419486>
…
lol
brisk brook
I think all Pydis projects use 3.9.5
celest charm
Is there anything wrong with changing typing.List to list and such? I'm doing a type hint fix
dry folio
if only i had nitro 😔 this is my favourite emote of this server
sturdy barn
if only i had nitro 😔 this is my favourite emote of this server
You can still use it if you dont have nitro i'm pretty sure
dry folio
You can still use it if you dont have nitro i'm pretty sure
nope, stickers are now behind the paywall
celest charm
nope, stickers are now behind the paywall
They are not
I don't have nitro
you can send stickers from this server without nitro
dry folio
oh wait
Get Nitro to use it everywhere.
...
sorry
my bad
lmao
omg
it was walrus that got behind the paywall lol
celest charm
so... is there anything wrong with switching to new-style annotations like list[str]?
brazen charm
It's mostly an unnecessary diff to dedicate a change to that, the typing classes won't be going anywhere for a while
But it may be worth mentioning somewhere that new code should use the builtin generics so the typing code is not seen as a prevalent style that should be used
tawdry vapor
I don't really mind the diff
I prefer the consistency and fewer imports
Though if you waited until 3.10, you could also get rid of unions and optionals in a single sweep.
fallen patrol
Though if you waited until 3.10, you could also get rid of unions and optionals ...
i mean, that's like 9 months out probably
(not 3.10, that's coming soon, but until all of the dependencies are tested well on it)
surreal veldt
is this the place to ask if python bot uses dpy 2.0?
celest charm
is this the place to ask if python bot uses dpy 2.0?
Yes, and also to answer that it does not (yet (I assume because it's not stable yet)) 🙂
stable mountainBOT
pyproject.toml line 20
"discord.py" = "~=1.7.3"```alr thank you
short snow
Regarding #dev-log message, I would be finishing off my current open PRs whenver I get time (they are waiting for reviews)
gritty wind
I think there was a mostly strong consensus either here or in #dev-core to not convert the typing annotations. Mind you, this decision came on the heels of the spring cleanup Pr, and those who were against it, were strongly against it
molten perch
Hey! Can sir-lancebot#801 be grabbed? It hasn't been approved, but it has an up for grabs tag.
dusky shoreBOT
static canyon
I believe @celest charm was looking into this
molten perch
Oh, okay then.
thorny obsidian
@green oriole I'm a bit confused by your suggestion. What benefit would duplicating the team name be when it's right next to it? That seems redundant and not necessary, especially if someone with a screen reader isn't just reading out the links but is instead having the page be read in order. I would go with the title attribute here or make each team name just link to the github and not have two columns.
green oriole
<@!407110650217627658> I'm a bit confused by your suggestion. What benefit would...
Yeah, duplicating the actual text would be silly, although we can give links title attribute without any impact on the rendered page. Like "Grand Gecko Repository" on the link saying "GitHub".
celest charm
Hey! Can sir-lancebot#801 be grabbed? It hasn't been approved, but it has an `up...
It's up for grabs once it's approved, that's what I meant
celest charm
I think there was a mostly strong consensus either here or in <#4112005996533514...
Is this about turning typing.List to list and such?
gritty wind
Yes
celest charm
why was that decision made? can you link?
short snow
So I think there is a bug in the helpdm, it removes the channel from user cache first and then it closes the channel. So if you put a message between the close command and closing if the channel, you would get a helpdm message
gritty wind
I can maybe link, give me a minute
celest charm
just curious, I'm not going to argue with core developers
tawdry vapor
You absolutely should, though
celest charm
I have a bad habit of arguing with people online for hours...
short snow
I can relate, I just had one with bast and Aru on a project we are working on
tawdry vapor
I was not part of that discussion, but I don't see why that decision should be final, especially if it was contentious to begin with.
gritty wind
I can’t even find the discussion at this point, we can talk about it again if y’all like
Here are my points from last time though
I remember an actual discussion though, not just this
short snow
So I think there is a bug in the helpdm, it removes the channel from user cache ...
Example
Help candy
tawdry vapor
That's a fair point in that context, but in this case, the PR is specifically for changing type annotations and includes no other changes. Unless you want to count fixing incorrect type annotations and changing List to list as two separate types of changes.
static canyon
So I think there is a bug in the helpdm, it removes the channel from user cache ...
That's just a race condition
short snow
That's just a race condition
Yeah but it could be ignored if we do that after closing the channel
gritty wind
The thing is, it’s a lot of small changes, spread out over a ton of files
So you’re either forcing 3+ people to go through every file for a small gain
Or relying 100% on find and replace
With the reviewers just clicking okay
static canyon
Yeah but it could be ignored if we do that after closing the channel
As far as the bot is concerned the user hasn't closed the channel though
celest charm
I already made the changes, because they are so tiny.
And I did not use find&replace
gritty wind
So was spring cleanup
Tiny changes
That doesn’t get rid of the point that a reviewer has to go through everything and make sure nothing is broken, missing, or anything else
short snow
As far as the bot is concerned the user hasn't closed the channel though
They have right? Let me get msg link
gritty wind
For what amounts to little benefit
celest charm
alright, let's not do that then
short snow
static canyon
They have right? Let me get msg link
They haven't closed the channel. They've sent a request, but it hasn't been closed
There's not really anything that can be done other than like sleeping or something and then seeing if it's closed
gritty wind
You can PR it at this point if you like
short snow
Umm I think I get it, thanks
gritty wind
But I’ll leave dealing with the mess to everyone else who wants this change
(P.S That last part is a joke, real reason is I don’t have a PC)
tawdry vapor
So when can this change happen (or any changes of a similar nature)? If we only apply it to new changes, then we end up with inconsistencies. If it's done as a kaisen when working in the same area, then again, inconsistencies plus it takes a very long time to migrate.
short snow
Wouldn’t a type checker help in reviewing?
gritty wind
It would, we discussed this here a bit higher up
short snow
Was that meant to me or mark?
gritty wind
So when can this change happen (or any changes of a similar nature)? If we only ...
You’re right, but the question becomes are these inconsistencies bad enough to warrant the effort
gritty wind
Was that meant to me or mark?
tawdry vapor
I argue that the effort is not as high as you make it out to be
short snow
I didn’t mean in that context, I just meant for reviewing this Pr, which changes the type hints
gritty wind
Well, currently you can’t actually run mypy on our code without a bunch of other errors you’ll need to work around
From converters to plain wrong type hints (those are more a problem with lance)
Both having been discussed up as well
brazen charm
is discord.py typehinted or some stubs exist?
green oriole
I'm sure we miss a ton of optional and proper null handling in a lot of places
There are third party stubs because Danny
tawdry vapor
It's not. Stubs exist as a PR but it's quite outdated. Maybe there is something else out there
celest charm
If type hints are so insignificant, and people just slap wrong typehints on functions and then pass the review, maybe we should just stop mandating type hints?
At least in seasonal-bot.
gritty wind
They’re not insignificant, and can significantly help keep some of the more complex systems a bit more easily digestible when used
And some of the plain wrong ones are very old and were being replaced in spring cleanup
Just not all of them
I believe now errors with type hints are very likely to be pointed out in new code
brazen charm
I argue that the effort is not as high as you make it out to be
I don't think the inconsistencies between the two "styles" are really that bad so I'd personally say it's not worth spending much time on, if any. But if someone's willing to do it then ¯_(ツ)_/¯
celest charm
well, I already made the changes, and we've spent more time arguing that me doing it (well... not really, but order is similar)
tough imp
I volunteer to review
short snow
Lol
tough imp
green oriole
If type hints are so insignificant, and people just slap wrong typehints on func...
We do correct them when they are plain wrong. We just use them as IDE tools, not for actual type-checking. So if they aren't 100% accurate it isn't worth the effort of generating a thousand lines diff to fix them.
tawdry vapor
Probably as much time as it would take to review them too :^)
green oriole
well, I already made the changes, and we've spent more time arguing that me doin...
You changed what? Sorry, I'm just jumping in the conversation
viscid coral
What's the difference really
celest charm
I volunteer to review
https://github.com/python-discord/sir-lancebot/pull/802
I sneaked in some other fixes (2 or 3) which should've been in a separate commit, but not sure if it's worth splitting now
celest charm
You changed what? Sorry, I'm just jumping in the conversation
typing.List -> list and such
gritty wind
What's the difference really
Practically nothing other than getting partially rid of the typing module, which is by no means a bad thing
tough imp
will the typing ones be deprecated?
green oriole
I don't think so
brazen charm
they are
green oriole
Okay nevermind me then
short snow
Oh GitHub got new org view
green oriole
But the concensus was to only change them when we edited the files, did you asked any core dev before working on it @celest charm ?
tawdry vapor
The deprecated functionality will be removed from the typing module in the first Python version released 5 years after the release of Python 3.9.0.
Not any time soon, but eventually
brisk brook
So it will be removed when Python 3.9 is abandoned/unsupported/frozen
short snow
Yeah
celest charm
But the concensus was to only change them when we edited the files, did you aske...
No, I just did the changes because they're so small. I didn't want to wait for a core dev approval
celest charm
yeah hopefully humanity is wiped from the Earth until 2025
green oriole
No, I just did the changes because they're so small. I didn't want to wait for a...
Eh, 300 lines diff isn't a small change. It would be nice if you could at least give a heads up here before.
celest charm
alright 🤷
gritty wind
You don’t need approval to work on something, it’s just you run the risk of wasting your time, and the time of reviewers
gritty wind
I think that’s been the view for quite a while
celest charm
Anyway... let's discuss something more important.
Does anyone want to grab this? https://github.com/python-discord/sir-lancebot/issues/801
And can the core devs review it?
apart from joe
green oriole
Yes, let's do that
celest charm
👍
tawdry vapor
I feel like this is being blown out of proportions, and I fail to understand why. I agree with the sentiment that we shouldn't do something that takes a significant effort but offers few benefits. However, this is a small PR and it's easy to review.
This sort of thinking is a larger issue; it's concerning because I feel it discourages making these sort of harmless small changes. Yes, it's low benefit, but it's also low effort contrary to what you're trying to portray. Conflicts are trivial to fix. Reviewing this probably won't take someone more than 15 minutes.
green oriole
To go back to 802 if we can merge it before the end of next week I'm fine with it. Although I would like to hear what other core devs think.
tough imp
I glanced over the PR and it should honestly be trivial to get merged
although the PR description also mentions transitioning to dataclasses which I havent seen in the diff yet
celest charm
@tough imp I changed the description now, let's keep it not that enthusiastic for now
that was something I was planning for, but seeing how it's apparently a controversial change let's not do a big refactoring
molten perch
Anyway... let's discuss something more important.
Does anyone want to grab this?...
I'm a bit lost in this sea of issues, if it's still up for grabs I'd love to grab it.
celest charm
I'm a bit lost in this sea of issues, if it's still up for grabs I'd love to gra...
Yes, it's up for grabs, and it's just been approved
go for it if you like 👍
ping me and I'll review the changes
green oriole
Cool! Could you leave a command on the issue @molten perch so I can assign you?
molten perch
Cool! Could you leave a command on the issue <@263957159807549450> so I can assi...
I've just left one! 🙂
green oriole
Sweet, thanks!
short snow
Btw are you working on the alembic setup for API? @molten perch
molten perch
Oh, well. I would, but in order to setup Alembic, maybe we should first setup the models, that's why I left a suggestion to easily do this. But haven't gotten any feedback yet.
green oriole
Heyo @hardy gorge! I don't think you have a lot of free time recently, but when you can if you could have a look at the API alembic issue ^ that'd be huge! 
short snow
Keep it a plain setup, I think we can add the models as we go on working
Uh I think I got it in the wrong context, lemme see your comment
green oriole
I don't think that's a good idea, having a good feel for the structure of the project is quite important early on
Having to refractor into modules midway into development is a PITA
gritty wind
I thought we were maintaining the old django models so we can move the tables
celest charm
Btw @molten perch, you should see how it's done in
https://github.com/python-discord/sir-lancebot/blob/main/bot/exts/halloween/scarymovie.py#L40-L41
And implement it like that, using aiohttp's built-in tools
short snow
No, what I meant was like we already have a got structure and we can add models when needed.
stable mountainBOT
bot/exts/halloween/scarymovie.py lines 40 to 41
# Get total page count of horror movies
async with self.bot.http_session.get(url=url, params=params, headers=headers) as response:```https://github.com/tiangolo/full-stack-fastapi-postgresql/tree/master/{{cookiecutter.project_slug}}/backend/app/alembic have a look at this
It would be empty as of now
Since we have no tables
gritty wind
I thought we were maintaining the old django models so we can move the tables
We don’t have tables?
green oriole
We have models?
molten perch
Btw <@!263957159807549450>, you should see how it's done in
<https://github.com...
Yes, I've checked out everything before actually volunteering 😄
gritty wind
I feel like I’m missing part of this plan
gritty wind
Well api is meant to be a replacement for our django api
With the same functionality
So theoretically you’d just use the same data structure?
short snow
Yeah ik, I am saying the current code has no tables as of now, so we have no models as of now, when we would be sifting feature one by one we can add them to dm and apply migrations
molten perch
We have models?
No we don't! But I have a solution for that matter, it's similar to reflecting the Database every time it starts up, except the library in my proposal would actually generate the models.
Oh, forgot to mention. It generates the tables based on the current database schema.
short snow
I had not do that, I had like to specify it my self, what I get is that you are wanting to dynamically generate models?
molten perch
Check out my comment: https://github.com/python-discord/api/issues/7 and the library, I mentioned.
You'd still get to modify the models, it'd just simply generate the most of it.
I mean.. we have nothing to lose 😄
short snow
I am not sure, but I had go with making them myself
molten perch
I don't really understand.. have you already done it? 🙂
short snow
Basically, I haven’t used your library yet and I think it is an overkill when one can make them on their own and it is pretty simple, the library functionality is like the models depend on the dB but it’s the other way round where you create the dB according to your models.
Yes I have used alembic a few times before this with FastApi, 2 times to be accurate, if that was your second question. We are already having the models we just need to "transfer" them.
molten perch
I mean.. you can still modify them, it would just generate the basic structure. Fields, and relationships based on the current schema.
It'd just make it a bit easier.
short snow
Still having Original opinions, what do others think?
cold island
Hmm.. poetry is giving me trouble with the site repo. It works just fine on other repos, but for the site it errors with the message error: Microsoft Visual C++ 14.0 or greater is required. Get it with "Microsoft C++ Build Tools": https://visualstudio.microsoft.com/visual-cpp-build-tools/ when installing libsass. I installed and re-installed that several times by now. Ideas?
tawdry vapor
You specifically installed build tools rather than visual studio?
cold island
You specifically installed build tools rather than visual studio?
yes
tawdry vapor
Have you tried updating setuptools and pip? Setuptools is what's ultimately trying to build it, right?
cold island
Tried now, still gives the error
I just tried installing libsass with no env, no issues
tawdry vapor
The same version?
tawdry vapor
I guess the other version already had a wheel
Or did you see that it had to build it?
cold island
Using legacy 'setup.py install' for libsass, since package 'wheel' is not installed.
Installing collected packages: libsass
Running setup.py install for libsass ... error
I mean when you successfully installed the other version?
cold island
Ah
It just said installing, and then successful
It had to download first if that's what you mean
tawdry vapor
Another idea: build tools comes with a command prompt that sets up an environment for using the build tools. Try installing the package inside that command prompt
There should be a shortcut called "x64 Native Tools Command Prompt for VS 2019"
cold island
Failed
tawdry vapor
I dunno
I used to have this exact issue and one day I installed build tools again and it just started working
Before, when I was desperate and didn't wanna use my linux VM, I would run a Windows VM and build the wheel on there, then transfer the file to the host. It would work fine on a fresh install of Windows.
cold island
I added libsass = "0.21.0" at the beginning of the dependencies list in pyproject.toml and apparently things are ok now? I don't even know lol
I then did poetry update instead of install and it didn't seem to install libsass so..
tough imp
while looking over seasonalbot's #802 I found some outdated docstrings & unused legacy config
I could PR a cleanup but I don't want to create another awkward cleanup PR
celest charm
I think there's also a cog that's literally not used... maybe we can just delete it?
green oriole
Yeah, although someone needs to actually fix it lol
gritty wind
I worked on it for a while, and I all have left to say is… don’t do latex on discord
green oriole
Fair advice haha
cold island
Is there no way to tell ahead of time how much memory it's going to try to allocate?
gritty wind
Gonna have to send someone back to math class
Jokes aside, iirc from my research back then, I didn't find any good way to do it
So being unable to calculate the required resources ahead of time, nor properly implement resource limits, I couldn't figure a way to keep it from crashing everything
(It also doesn't help that looking up info on this subject usually gives you more math or general results)
olive cloak
Probably the wrong place to ask but does the python server statistics bot update the database with every single message sent?
If the servers busy that's a lot of querying lol
Ohhh is the dB in memory?
tawdry vapor
Yes, it does update for every message
The data is sent to graphite, which is responsible for saving the data
olive cloak
Hmm graphite, gotta look into that, thanks
tawdry vapor
Graphite uses this as its database. It's specialised for this sort of use case https://graphite.readthedocs.io/en/latest/whisper.html
cold island
Do you mean the stats collected from the Python bot, or Metricity?
Metricity uses pg
short snow
stats collected by the bot is only the help channel usage, right? which uses graphite
green oriole
We actually collect a ton of metrics
I know at least infractions are from the bot
Slowmode also is
short snow
yeah i just saw
the error handler, python news, codeblock_corrections, rules, voice get, token
green oriole
Errors, filters, #mailing-lists, defcon, codeblock correction, webhooks, voice gating, rules usage, PEP usage, removed tokens, tag usage, mod alerts and documentation
That's all the basic usages I could find
short snow
ah nice
green oriole
I feel like some of our dashboards are directly querying the db, I don't see where stats on infractions are collected (CC @patent pivot)
fervent sage
we can always do it like mathbot does and just render via api and post - https://github.com/DXsmiley/mathbot/blob/master/mathbot/modules/latex/__init__.py
austere lantern
can somebody tell me what the developement section is all about? Newbie here...
static canyon
can somebody tell me what the developement section is all about? Newbie here...
This channel is for our open-source projects like @dusky shore and @stable mountain
dusky shoreBOT
stable mountainBOT
austere lantern
This channel is for our open-source projects like <@!528937022996611082> and <@!...
ohh ok! thank you.
how do i start contributing to these projects?
static canyon
But basically take a look at the issues and see if there's one you'd like to do
Or take a look at the PRs and see if there's anything you feel you can review (although your review won't count towards the mandatory count since you're not staff/core-dev)
austere lantern
thanks a lot!
patent pivot
I feel like some of our dashboards are directly querying the db, I don't see whe...
yeah a few have a RO user on Postgres
short snow
A important PR on quackstack is needing a review, https://github.com/python-discord/quackstack/pull/52 if anyone has got time 😄
And, could somone merge this https://github.com/python-discord/quackstack/pull/57
got its approve ^
short snow
this is not really a "dev" topic, but rather about how to play the game.
Isn't 2 3 0 a valid choice? 2 has 1 card, 3 has 2, and 0 has 3?
timid sentinel
2 and 3 both have no hat
and 2 and 0 both have a sword
you have to find three with no matching characteristics at all
Like 6 1 2 I think
timid sentinel
Are there any others there?
I don't think so
oh wait
ok I only just understood what it means by "3 cards where each feature is either all the same or all different", I guess that's per feature, so you can have three of the same colour but everything else different for example
that makes more sense
short snow
Yeah, a few more
(3, 9, 10)
(2, 4, 8)
(1, 2, 6)
(1, 5, 9)
(4, 5, 11)
stable mountainBOT
Dockerfile line 1
FROM python:3.9.5-slim```
`Dockerfile` line 1
```Dockerfile
FROM python:3.9-slim```feel free to submit a PR
timid sentinel
I'd say just 3.9-slim would be fine
patent pivot
yeah, 3.9 slim is fine
fallen patrol
no, I know
fallen patrol
stable mountainBOT
.github/workflows/lint-test.yml line 46
python-version: '3.9'```Cool
fallen patrol
none of lint_test.y[a]ml in bot, site, lance are pinned to a patch version of 3.9
https://github.com/python-discord/bot/blob/091417c2299b23221aa02212f60e6d8f7bfaf2c4/.github/workflows/lint-test.yml#L46
https://github.com/python-discord/sir-lancebot/blob/75fe0f541465979fecf197590aa37cae21a61479/.github/workflows/lint.yaml#L41
https://github.com/python-discord/site/blob/4de9cdf66e6ea6c5ac736dbc99551565be5bdcd5/.github/workflows/lint-test.yaml#L38
stable mountainBOT
.github/workflows/lint-test.yml line 46
python-version: '3.9'```
`.github/workflows/lint.yaml` line 41
```yaml
python-version: '3.9'```
`.github/workflows/lint-test.yaml` line 38
```yaml
python-version: '3.9'```@patent pivot oh now i see why @radiant merlin is using a url instead of a git link for discord py 2 lol
git is not installed in 3.9.6-slim
patent pivot
git is not installed in most distros
I had to install git on a machine today
sadge
fallen patrol
I had to upgrade git on all of my machines
since it wouldn't support changing the default branch
2.21.2 to 2.32.0 lol
i remember you saying things would work then we finally checked that value
sleek steppe
@short snow #dev-log message I didn't change any logic for the automatic linking of issues in the PR. I was thinking about changing that part but then I thought it wouldn't make sense if there were multiple different users and it only picking one
viscid coral
Anyone wants to give their opinion about bot#1717 ?
dusky shoreBOT
celest charm
@tough imp I reviewed your review 
tough imp
okay 😌
tough imp
i'm sensing a strong LGTM on this one
short snow
<@736234578745884682> https://discord.com/channels/267624335836053506/6228953251...
Ah right, maybe just remove that completely, no hyperlinks?
trim cradle
Someone just tried to do .bm in #internals-and-peps and it said "you can only use this command in #sir-lancebot-playground"
this seems wrong
green oriole
Yeah I guess now we have replies bookmark merged we can allow it everywhere
You are supposed to bookmark by ID though
trim cradle
You are supposed to bookmark by ID though
.bm 874008100510761031 for further reference :DDDD was the invocation; it wasn't a reply
green oriole
Ah well, for most commands it reduce clutter to do them in another channel, but it seems fine here
cold island
Yeah I wouldn't mind being more liberal with that one
vale ibex
Anyone around to turn on a bot in the test server?
Need to test my bot against talent pool from @stable mountain
brisk brook
How can I help
brisk brook
What do you want me to do with it?
vale ibex
Nothing, just have it running 😄
I testing a thread bot for the nomination cog
so I just need a python bot running that I can interact with
can't run my own since my bot token is running this thread bot
brisk brook
It's not happy with me, keep complaining about Redis. Even though I use Docker
Ah, I'll just go with fakeredis
vale ibex
Have you seen the config.yml on notion?
brisk brook
I am here, getting through these small steps. https://pythondiscord.com/pages/guides/pydis-guides/contributing/bot/
vale ibex
Ah, we made it easy for the test server 1 sec
brisk brook
Oooh, okay haha
celest charm
@brazen charm @tawdry vapor
Check this out, I made a custom version of "Annotated" that doesn't require patching anything https://gist.github.com/decorator-factory/718a6b6bf514c496baf385f0c0cfcdaa. It also makes sure the argument type is actually correct. Unfortunately it doesn't work with mypy, only pyright. Do you think this is a direction we could look into?
tawdry vapor
<@!184351770636582913> <@!137073073168973824>
Check this out, I made a custom v...
It's a neat idea but lack of cross-compatibility kills the idea
celest charm
It's a neat idea but lack of cross-compatibility kills the idea
well, my question is whether we could find another variation that works in both
tawdry vapor
I'm not particularly worried about Annotated having the wrong type, since the idea is to have aliases for all them. Therefore, they will only be defined once. There is still some risk of them falling out of sync with the actual converter, but it's not that likely.
celest charm
yeah, perhaps
tawdry vapor
If you can find something more robust though, then by all means we can use it
celest charm
A purely type-level solution also has the advantage that it doesn't need the monkeypatching, which could get out of sync with later versions of discord.py
celest charm
so I'll try to look for a compatible solution
tawdry vapor
Also keep in mind many use PyCharm which is neither mypy nor pyright but its own thing
celest charm
right... it's a mess
I'm not sure whose fault it is, the spec or the type checkers, but the spec is interpreted really differently by them
tawdry vapor
Is Annotated subclassable?
celest charm
celest charm
The 200 IQ move would be to find a way to do
if MYPY:
...
else:
...
Yeah I'm definitely for some "proper" solution but missing out on pycharm/mypy is too big of a downside for what you have now
celest charm
I'm not saying we should use this particular hack, it was more of an illustration that this could be in theory solved without monkeypatching
I really wish Python's type system got more stable and rich. Languages like Haskell or TypeScript have useful libraries that mostly consist of type definitions.
tawdry vapor
Where do other type checkers fail?
Do they just not care about the returned value of getitem?
Do they not support a generic getitem perhaps?
Have you tried __class_getitem__?
celest charm
Where do other type checkers fail?
brisk brook
I really wish Python's type system got more stable and rich. Languages like Hask...
Yes 🙏
I just want to be able to type something as returning of the same type as self ;-;
tawdry vapor
Hmm really sounds like mypy would prefer __class_getitem__
But that only takes 1 param so maybe your whole concept breaks down
brisk brook
No, it would take 1 parameter, that parameter being a tuple?
tawdry vapor
Oh no, so does getitem
celest charm
Hmm really sounds like mypy would prefer `__class_getitem__`
__class_getitem__ doesn't seem to work with either
tawdry vapor
Are you doing it on the metaclass or the actual class?
celest charm
The actual class ```py
class Arg:
def class_getitem(cls, arg: tuple[type[T], type[ConverterLike[T]]]) -> type[T]:
actual_type, converter_class = arg
return converter_class # type: ignore
What Python is missing here is type-level functions.
It's, like, the best feature in TypeScript
brisk brook
What Python is missing here is type-level functions.
Huh?
tawdry vapor
I can't seem to get mypy to work at all with class_getitem
No matter how I define it, it never thinks it accepts any type args
Not even 1
brazen charm
Is the generic getitem a typing or oyright thing? From googling around a bit I didn't really find anyt about it but my typing knowledge is not very deep
tawdry vapor
celest charm
Huh?
In TypeScript, you can do stuff like
class MyContainer<T> {
// ...
}
type ElementOf<T> =
T extends (infer X)[]
? X
: T extends MyContainer<infer X>
? X
: never;
``` and then `ElementOf<number[]>` becomes `number`, and `ElementOf<MyContainer<string>>` becomes `string`Nice haha
brazen charm
It's standardised https://docs.python.org/3/reference/datamodel.html#object.__cl...
Yes, but is that just that it should return an object representing that generic (obj[t] returning some object representing that), or that checkers actually should look into it to understand what's going on?
tawdry vapor
The actual class ```py
class Arg:
def __class_getitem__(cls, arg: tuple[type...
It seems like mypy wants it to subclass typing.Generic in order for it to understand that it takes type arguments.
celest charm
hmmmm yeah
But if we make it generic, we can't specify a relation between the generic variables
tawdry vapor
Someone did try to use it without subclassing Generic in this issue https://github.com/python/mypy/issues/7319
Not sure if it implies it's possible, or if it was a poorly thought out example
same issue we have https://stackoverflow.com/questions/62410076/how-to-use-getitem-on-a-class-while-keep-typing-mypy-happy
celest charm
One day we will give up and rewrite @stable mountain in Agda.
tawdry vapor
Pydantic is managing to do it, though it's not generic https://github.com/samuelcolvin/pydantic/blob/master/pydantic/generics.py#L44
stable mountainBOT
pydantic/generics.py line 44
def __class_getitem__(cls: Type[GenericModelT], params: Union[Type[Any], Tuple[Type[Any], ...]]) -> Type[Any]:```use a string annotation lol
vale ibex
lol that's a good question
my answer is I wrote it late last night and didn't think about it
we'll probably make it public at some point, don't see a reason not to
it's nothing exciting though, just us messing with threads
It's actually only got 22 lines of new code
the rest is borrowed boiler plate lol
short snow
👍
vale ibex
I've setup the webhook now too, just waiting for an owner to be available to set it to public
willow sluice
async def command_not_found(self, string: str) -> "HelpQueryNotFound":
"""
Handles when a query does not match a valid command, group, cog or category.
Will return an instance of the `HelpQueryNotFound` exception with the error message and possible matches.
"""
choices = list(await self.get_all_help_choices())
result = process.extract(default_process(string), choices, scorer=fuzz.ratio, score_cutoff=60, processor=None)
return HelpQueryNotFound(f'Query "{string}" not found.', {choice[0]: choice[1] for choice in result})
why do default_process(string) and processor=None when the default processor is default_process ?
(I wanted to do something like the Python bot's "similar commands" and I came across this, and I'm wandering why and if should do the same in my code...)
vale ibex
It used to be ```py
if full_process(string):
result = process.extractBests(string, choices, scorer=fuzz.ratio, score_cutoff=60, processor=None)
else:
result = []
where we'd process the string before hand to avoid matching if processed string is empty
It's been refactored and changed since then
so I think the way it is now is just because during some recent refactoring we overlooked what it was we were doing exactly
in favour of keeping logic similar
timid sentinel
I was wondering the same thing when looking at the bot PR before I did https://github.com/python-discord/sir-lancebot/pull/799
and my plan worked lmao, thanks Akarys
vale ibex
lol
green oriole
I was wondering why the error thing, but it is just a new lint rule. Quite a weird one tbh
vale ibex
Anyone know why I'm having issues with this predicate not short circuiting? ```py
def predicate(message: discord.Message) -> bool:
return all((
message.reference,
message.reference.message_id == message_id,
isinstance(message.channel, discord.Thread)
))
thread-bot | File "/bot/bot/exts/nominations.py", line 38, in predicate
thread-bot | message.reference.message_id == message_id,
thread-bot | AttributeError: 'NoneType' object has no attribute 'message_id'
my understanding that is if message.reference resolves to false, then it short circuits, and doesn't check the next line
but this error suggests that it is?
brazen charm
you're creating a tuple first
thorny obsidian
the tup-- numerlor beat me to it
vale ibex
I thought all requires an iterable?
brazen charm
it does, but a tuple will have to be constructed whole. If you lose one set of parentheses you'll turn it into a lazy generator that should short circuit as you expect it to
vale ibex
ahh right
def predicate(message: discord.Message) -> bool:
return all(
message.reference,
message.reference.message_id == message_id,
isinstance(message.channel, discord.Thread)
)
still raises the same error
Added logger.info(bool(message.reference)) just above the all for my sanity
it logs false
brisk brook
What exactly is this matching?
vale ibex
discord.utils.find(predicate, self.bot.cached_messages)
brisk brook
You want to find the message that a thread was created in?
vale ibex
I want to find the thread linked to the given message ID
brisk brook
This message ID is in the thread, or started it?
Instead of checking message reference, check message flags
vale ibex
how would that work?
given a message_id, I want to get the Thread object that is it's child
brazen charm
still raises the same error
ah nevermind, it won't create a generator in here, not sure why I thought so. It's trying to pass in the argument (which would fail anyway) so it errors out anyway.
I'm not sure how you'd do it with your above condition with just all, but chaining ands shouldn't look too bad
gritty wind
Not any nots 🤡
vale ibex
ah nevermind, it won't create a generator in here, not sure why I thought so. It...
Ah, that makes sense now
I'm glad I wasn't missing something
vale ibex
This message ID is in the thread, or started it?
Yea, it's the "outside" message ID
I'm listening for on_raw_message_delete events and am checking if the delete message is the "parent" of a thread, if it is, I'm archiving the thread
async def get_thread_from_message_id(self, message_id: int, channel_id: int) -> t.Optional[discord.Thread]:
"""Returns the thread linked to the given message id."""
def predicate(message: discord.Message) -> bool:
"""Returns true if `message` is the thread_starter_message for the `message_id`."""
return (
message.reference
and message.reference.message_id == message_id
and isinstance(message.channel, discord.Thread)
)
if message := discord.utils.find(predicate, self.bot.cached_messages):
logger.info("Cache hit!")
return message.channel
logger.info("Message not found in cache, checking all threads...")
channel: discord.TextChannel = self.bot.get_channel(channel_id)
for thread in await channel.active_threads():
messages = await thread.history(limit=1, oldest_first=True).flatten()
if messages[0].reference.message_id == message_id:
return thread
return None
beautiful
brisk brook
A few things, there should be a nullable "cached message" attribute in the on_raw_message_delete field.
vale ibex
That would give me the Message object for that yea, but there isn't an easy way to go from that to the child Thread object
brisk brook
So you can find the cached message (if it exists). Then use https://github.com/Rapptz/discord.py/blob/master/discord/flags.py#L283
stable mountainBOT
discord/flags.py line 283
def has_thread(self):```So that returns true if the message has a child thread?
celest charm
ahh right
ez solution ```py
def predicate(message: discord.Message) -> bool:
@all
@lambda f: f()
def _():
yield message.reference
yield message.reference.message_id == message_id
yield isinstance(message.channel, discord.Thread)
return _
yo wtf
celest charm
Actually, a @predicate decorator could be cool
@predicate
def is_in_thread(message: discord.Message) -> bool:
yield message.reference
yield message.reference.message_id == message_id
yield isinstance(message.channel, discord.Thread)
remote wigeon
welcome to python fuckery
brisk brook
@staticmethod
async def check_first_message_referencing(thread: discord.Thread, message_id: int) -> t.Optional[discord.Thread]:
messages = await thread.history(limit=1, oldest_first=True).flatten()
if messages[0].reference.message_id == message_id:
return thread
async def get_thread_from_message(self, message: t.Union[discord.Message, int], channel_id: int) -> t.Optional[discord.Thread]:
"""Returns the thread linked to the given message."""
if isinstance(message, discord.Message):
if not message.flags.has_thread:
return None
message_id = message.id
else:
message_id = message
# Find the thread start message in cache
for msg in self.bot.cached_messages:
if msg.reference and msg.reference.message_id == message_id:
return msg.channel
# The message cache can be big, we don't want to block the bot while computing this
await asyncio.sleep(0) # Explicitly yield
channel: discord.TextChannel = self.bot.get_channel(channel_id)
for thread in channel.threads: # Thread may be in cache
if found := await self.check_first_message_referencing(thread, message_id):
return found
# Last attempt, request all open threads
for thread in await channel.active_threads():
if found := await self.check_first_message_referencing(thread, message_id):
return found
# There is no thread associated with this message
return None
Here's the best I could do, little to no improvement
The only improvement is first checking the threads in memory, as well immediately returning if the message was in cache and doesn't have a thread. CC @vale ibex
vale ibex
I guess I could do that after my predicate, before I return the channel
Return none if the flag isn't there
brisk brook
About the asyncio.sleep(0), see https://github.com/Rapptz/discord.py/issues/6700
vale ibex
Hmmm, using a find like I am seems pointless actually looking at your code
brisk brook
The message cache lookup can block enough to be troublesome, considering this server is pretty big and many message deletes
vale ibex
Since the message delete has a reference to the cached message already
vale ibex
yea
brisk brook
But we need to find the right message
That one is considered the thread starter message, and is actually in the thread
vale ibex
yeaa
So I can exit early if the cached message delete doesn't have a thread
the find is probably still needed after that
brisk brook
The message cache lookup *can* block enough to be troublesome, considering this ...
Continuing on this, I would guess that this action is pretty low-prio. So yielding after each check I would say is okay?
Otherwise we could use enumerate and use the modulo operator to only yield "every now and then"
brazen charm
considering how inexpensive each op is, I'd personally group them and only yield control every few hundred iterations
brisk brook
the find is probably still needed after that
Yup, so we go like this:
-
If message is cached, and does not have a thread -> return immediately (good escape 😉 )
-
Check cached messages for the thread starter message, if found grab its channel (a thread)
-
Check in-memory threads and fetch the first message in each looking for the correct thread starter message
-
Fetch all active threads and fetch the first message in each looking for the correct thread starter message
The bottom two may overlap, not sure how we would solve that
brisk brook
considering how inexpensive each op is, I'd personally group them and only yield...
Yeah very true, didn't want to complicate the code when typing it out here as a proposal
vale ibex
I am assuming sending a message in a thread puts that thread back into the cache
if that's the case, I'd imagine the thread will be in the cache 90% of the time for this specific use case
async def find(predicate: Callable[[T], Any], seq: Iterable[T], chunk_size: Optional[int] = 0) -> Optional[T]:
"""
A helper to return the first element found in the sequence that meets the predicate.
If chunk_size is specified, find will yield control between each chunk.
"""
for index, element in enumerate(seq, 1):
if predicate(element):
return element
if chunk_size and index%chunk_size == 0:
await asyncio.sleep(0)
return None
smth like this?
vale ibex
I was removing an unnecessary check
starting the enumerate at 1 means it won't yield on the first iteration
brisk brook
Actually no we're good yeah, it's only if the.. second part of the modulo operator is 0
>>> 0 % 0
Traceback (most recent call last):
File "<stdin>", line 1, in <module>
ZeroDivisionError: integer division or modulo by zero
>>> 0 % 1
0
ye
brisk brook
starting the enumerate at 1 means it won't yield on the first iteration
Yeah so no need to do this since you don't get zero division error from that
vale ibex
we don't but it will yield on the first iteration right if we do?
brisk brook
Ah, true
async def find(predicate: Callable[[T], Any], seq: Iterable[T], chunk_size: Optional[int] = 0) -> Optional[T]:
"""
A helper to return the first element found in the sequence that meets the predicate.
If chunk_size is specified, find() will yield control to the event loop between each chunk.
"""
# Start at 1 so that we don't unnecessarily yield on the first iteration
for index, element in enumerate(seq, 1):
if predicate(element):
return element
if chunk_size and index%chunk_size == 0:
await asyncio.sleep(0) # Yield to the event loop
return None
These changes and it's golden imo ^
brisk brook
Actually, there's an issue in the typing?
vale ibex
I copied the typing from the Discord find func lol
brisk brook
chunk_size: Optional[int] = 0, Optional[T] means Union[T, None].
It should just be chunk_size: int = 0 I think
Could also get away with making it a kwarg-only because it may not be very clear to someone what find(predicate, messages, 100) does.
vale ibex
Yea, I'll make it a kwarg
brazen charm
I think something like
for chunk in more_itertools.chunked(it, chunk_size):
for elem in chunk:
...
if chunk_size:
await asyncio.sleep(0)
would be clearer
vale ibex
hah ofc there's an itertools func for it
vale ibex
I think something like
```py
for chunk in more_itertools.chunked(it, chunk_size)...
what would be the behaviour of chunked if the size is 0?
would it not chunk at all, or not iterate?
I'm going to guess the latter?
brazen charm
not iterate I believe, None should work
vale ibex
Ah, it does
Nice
async def chunked_find(predicate: Callable[[T], Any], seq: Iterable[T], *, chunk_size: Optional[int] = None) -> Optional[T]:
"""
A helper to return the first element found in the sequence that meets the predicate.
If chunk_size is specified, chunked_find() will yield control to the event loop between each chunk.
"""
for chunk in more_itertools.chunked(seq, chunk_size):
for element in chunk:
if predicate(element):
return element
if chunk_size:
await asyncio.sleep(0) # Yield to the event loop
return None
Looks good
yup, this logic works nicely
if the orignal message is cached, it fetches from there, if not, and there has been a message in the thread since the bot started, then it's in the thread cache
and if all fails the fetching works well
viscid coral
```py
@staticmethod
async def check_first_message_referencing(thread: di...
Just when I thought I know little about discord.py, here comes some code I would never think about understanding
I mean every code sent here, I don't understand lol, discord.py is really hard I wish I knew more about it
vale ibex
!paste
stable mountainBOT
brisk brook
I mean every code sent here, I don't understand lol, discord.py is really hard I...
Well, how can I help? If you read the code line-by-line, where do you get lost?
vale ibex
https://github.com/python-discord/thread-bot/pull/1 once joe makes it public tonight 😄
viscid coral
Well, how can I help? If you read the code line-by-line, where do you get lost?
Actually, I just read it twice line-by-line and understood it lol, thank you
celest charm
```py
async def find(predicate: Callable[[T], Any], seq: Iterable[T], chunk_size...
s/chunk_size: Optional[int]/chunk_size: int
oh, nvm
what about also allowing an async iterable?
I guess it's good enough here
dim pelican
Question: is it good practice to make a new branch for every Pull Request? I am doing some prep work for making a Real Python search command and just want to make sure that everything is done right. (Just took me too long to get Sir-Lancebot running again on a local Docker container)
celest charm
Question: is it good practice to make a new branch for every Pull Request? I am ...
Yes, if you're making something that's "a new feature", make a branch
btw, can you link the PR?
celest charm
ah
I just wanted to make sure you brush your teeth format URLs in a safe way, as this is now my kick apparently @dim pelican
dim pelican
I saw the recent PRs on the URL formatting, how is this:
...
params = RP_PARAMS | {"q": user_search}
async with self.bot.http_session.get(url=BASE_URL, params=params) as response:
...
👍
dim pelican
Woo! First Issue / Branch / PR for the discord server 🤞
celest charm
@dim pelican I left a few comments
# is this OK for showing the URL link?
Btw, don't leave comments like that in code. If you want to ask how to accomplish something, you can ask in this channel.
vale ibex
FWIW, thread-bot is now public https://github.com/python-discord/thread-bot
green oriole
green oriole
Until we have the results I think so (CC @thorny obsidian)
molten perch
Hey! As for api#7 should I open a new issue to keep things consistent?
dusky shoreBOT
fervent sage
could i make a quick pr to remove a few of the newlines here, its quite a chonky tag
viscid coral
could i make a quick pr to remove a few of the newlines here, its quite a chonky...
Maybe show the output you want it to look like first?
cold island
Hey! As for api#7 should I open a new issue to keep things consistent?
Sebastiaan isn't available at the moment to answer it, but if you think it's worth opening an issue for then feel free to do so, worst case it will be closed.
molten perch
Alright, thank you!
short snow
Hey! As for api#7 should I open a new issue to keep things consistent?
yeah that would be better
molten perch
api#16 Here it is.
short snow
Maybe add a line discussed in #6
green oriole
could i make a quick pr to remove a few of the newlines here, its quite a chonky...
Please do
fervent sage
done! bot#1733
dusky shoreBOT
dusky shoreBOT
green oriole
Heyo @patent pivot we broke tag speedrun
It is an and where it should be an or
I can have a look in like 5h if you aren't avaliable
fervent sage
uh oh
fervent sage
i tried to comment on this and gh is 500ing
vale ibex
!ext r help_channels
stable mountainBOT
:ok_hand: Extension successfully reloaded: bot.exts.help_channels.
vale ibex
!ext r help_channels
stable mountainBOT
:ok_hand: Extension successfully reloaded: bot.exts.help_channels.
gritty wind
!ext r help_channels
stable mountainBOT
:ok_hand: Extension successfully reloaded: bot.exts.help_channels.
gritty wind
Wanted to get in on the fun
vale ibex
odd. Needed 1 reload to get the 3 channels, and a second to refresh the channel list in #❓｜how-to-get-help
vale ibex
There was 1 channel available
After the first reload, there was 3, but one the two new channels were in #❓｜how-to-get-help
After the second, the message updated
I think it's some logic error somewhere
it looks like it's caused by a KeyError actually
after reboot, when a channel is opened this is getting hit ```
2021-08-10 16:58:40 KeyError: <TextChannel id=770709059732504586 name='help-cheese' position=189 nsfw=False news=False category_id=696958401460043776>
2021-08-10 16:58:40 self.available_help_channels.remove(message.channel)
2021-08-10 16:58:40 File "/bot/bot/exts/help_channels/_cog.py", line 151, in claim_channel
Which I am guessing is causing a new channel not to be pulled up
fervent sage
tag suggestions go on the meta repo now right?
brisk brook
Yes
green oriole
Hmmmmm they could go on the bot now @brisk brook @fervent sage
It will probably be easier this way
brisk brook
No, we agreed in the last staff meeting that all content go to meta
gritty wind
It just makes sense ™️
green oriole
go make an announcement 
brisk brook
We needed a use for the meta repo
viscid coral
What is meta repo
fervent sage
that should probably be a #dev-announcements thing actually so everyone knows to post them on meta
brisk brook
What is meta repo
The metaclass for the Python class
Tl;Dr any organization issues go there @viscid coral
green oriole
viscid coral
Tl;Dr any organization issues go there <@!480404983372709908>
I be stupid now, what do you mean organization issues?
There my nickname says it all
green oriole
Issues that concerns the organization, pydis, as a whole, not just one project in particular
brisk brook
I be stupid now, what do you mean organization issues?
Look at the issues on the repository, I think that will make the most sense.
viscid coral
lmao
mint nebula
vocal prairie
I don't believe that that was approved yet, but you can comment on the issue saying that you want to work on it, and than wait for a core dev to approve (or deny) it
green oriole
I will discuss it with the rest of the core devs and get back to you
viscid coral
How can you even become a core-dev?
green oriole
You need to be part of the staff first, and a core dev can put up a nomination for you if they feel like you are deeply involved in our projects
viscid coral
ah ok, thanks
static canyon
Lemon recently said in #community-meta about changing the DORMANT_MSG so that it dynamically sends the correct name of the categories. I'd like to implement this, and am thinking that it's just changing the DORMANT_MSG to a string and then doing .format() on it when it gets sent. https://github.com/python-discord/bot/blob/main/bot/exts/help_channels/_message.py#L32-L41 https://github.com/python-discord/bot/blob/main/bot/exts/help_channels/_cog.py#L390
stable mountainBOT
bot/exts/help_channels/_message.py lines 32 to 41
DORMANT_MSG = f"""
This help channel has been marked as **dormant**, and has been moved into the **Help: Dormant** \
category at the bottom of the channel list. It is no longer possible to send messages in this \
channel until it becomes available again.
If your question wasn't answered yet, you can claim a new help channel from the \
**Help: Available** category by simply asking your question again. Consider rephrasing the \
question to maximize your chance of getting a good answer. If you're not sure how, have a look \
through our guide for **[asking a good question]({ASKING_GUIDE_URL})**.
"""```
`bot/exts/help_channels/_cog.py` line 390
```py
embed = discord.Embed(description=_message.DORMANT_MSG)```That becoming something like```py
DORMANT_MSG = f"""
This help channel has been marked as dormant, and has been moved into the {}
category at the bottom of the channel list. It is no longer possible to send messages in this
channel until it becomes available again.
If your question wasn't answered yet, you can claim a new help channel from the
{} category by simply asking your question again. Consider rephrasing the
question to maximize your chance of getting a good answer. If you're not sure how, have a look
through our guide for asking a good question.
"""and thenpy
available_category = self.bot.get_channel(constants.Categories.help_available)
embed = discord.Embed(description=_message.DORMANT_MESSAGE.format(channel.category.name, available_category.name, ASKING_GUIDE_URL))```
viscid coral
Yap
static canyon
Wasn't sure if I'd need to handle constants.Categories.help_available not being in channel cache but doesn't look like it's handled in most places elsewhere so guess not
viscid coral
@static canyon wait why didn't you use str().format ?
static canyon
I am
_message.DORMANT_MESSAGE.format(...)
Can't do it directly where I define DORMANT_MSG because the info isn't in that file
viscid coral
ah ok
static canyon
We now have bot#1738
dusky shoreBOT
static canyon
Heh
Apparently flake8 doesn't support .format kwargs so it says undefined name
God damn it
No clue how to fix that
Oh wait
viscid coral
damn linter
static canyon
I still have the f in front of it lol
nvm lol
What did you mean by your GuildChannel comment? @viscid coral
viscid coral
GuildChannel have category attr the can also return None
Like some way to handle the constants help channel not cached
static canyon
I'm not following lol
vale ibex
stable mountainBOT
bot/utils/channel.py lines 56 to 66
async def try_get_channel(channel_id: int) -> discord.abc.GuildChannel:
"""Attempt to get or fetch a channel and return it."""
log.trace(f"Getting the channel {channel_id}.")
channel = bot.instance.get_channel(channel_id)
if not channel:
log.debug(f"Channel {channel_id} is not in cache; fetching from API.")
channel = await bot.instance.fetch_channel(channel_id)
log.trace(f"Channel #{channel} ({channel_id}) retrieved.")
return channel```👍
static canyon
Can I please get some reviews on bot#1738? It's just a quick-fix PR
dusky shoreBOT
viscid coral
I know my review is not helpful, but how can I review again?
I reviewed, but again, not helpful
dim pelican
I reviewed, but again, not helpful
I get the same feeling when I leave a review lol
static canyon
lol
static canyon
I reviewed, but again, not helpful
It's still helpful to know there's nothing blaringly obvious I missed that didn't get caught by linting
vale ibex
reviewed 🙂
I think this is caused by the fact that we call the http bulk channel update directly in move_to_bottom to edit the channel position, which doesn't update the channel in cache
vale ibex
I guess the fix for this is to just hard code that to the dormant category, similar to what we have for the in use category, rather than get it from the channel obj
or you can edit move_to_bottom to also update the channel object in the cache with the position change, but that's going to be more effort
and may have side effects elsewhere
static canyon
and may have side effects elsewhere
Yeah, it would
Actually maybe not
It shouldn't do from the looks of it, but it's easier to just try_get_channel anyway
vale ibex
it would be a case of looking at all the places that move_to_bottom is used, and checking if they rely of the channel object not being updated
for some odd reason
try_get_channel will still pull from the cache first
which isn't updated with the pos change
static canyon
I mean could we just make move_to_bottom return the new channel object?