async def bookmark(self, ctx: commands.Context, jump_url: str = None, *args) -> None:
#dev-contrib
clever wraith
green oriole
He just join them after, so I don't think so
green oriole
That's a seasonalbot generated message too
green oriole
You can temporary add a print here https://github.com/python-discord/seasonalbot/tree/master/bot/seasons/evergreen/error_handler.py#L58 but you should try gdude method first
clever wraith
yes ?
green oriole
Switching to the message converter and deleting the footer
green oriole
Told you, because it reduce the number of API call, make it faster and allow more things
clever wraith
that code is alredy in ^^
green oriole
Just change jump_url: str to message: discord.Model and get the jump_url from message.jump_url
And you can get rid of the history loop
clever wraith
we actually can't do it
green oriole
You actually can?
clever wraith
it will then return .bm message
glass pecan
Akarys is talking about adding it in the sig and referencing the msg from the argument as it'll already have been converted. Not to use the ctx.message object
Why would you be getting muted 
glass pecan
guilty concious?
lol
the suggestion is the change the bookmark commands function to this
@commands.command(name="bookmark", aliases=("bm", "pin"))
async def bookmark(self, ctx: commands.Context, message: discord.Message = None, *args) -> None:
discord.Message being typehinted allows all jump urls to work and also ensures it checks internal cache first
less api usage
green oriole
@commands.command(name="bookmark", aliases=("bm", "pin"))
async def bookmark(self, ctx: commands.Context, message: discord.Message = None, *, hint: str) -> None:```actually :)TBH i prefer the current version , no offense
glass pecan
you can prefer it, but we don't
clever wraith
getting message from cache can lead up taking message from other channel
green oriole
That's the point actually
hardy gorge
I think you misunderstrood what I meant earlier, AG
glass pecan
the argument provided is a jump url. it's unique to a single message
green oriole
IDs are (almost) unique too
glass pecan
they are always unique
hardy gorge
All messages have unique IDs, even if it's just the ID. However, the converter can even convert an ID to a Message object of a message sent to another channel if the message is still in cache.
So, it's a plus, not con
glass pecan
an automatically created general channel has the same ID as the guild on purpose
green oriole
Actually two messages in different channels can technically have the same ID well, I thought I read that somewhere, my bad
clever wraith
but we are not taking message ID by any ways
glass pecan
no they cannot akarys
clever wraith
they can
hardy gorge
The snowflakes are specifically designed to be unique
The last part of it depends on the worker ID + the internal worker counter
glass pecan
each process generating IDs have designated identifiers that don't allow overlaps
it's why they're called snowflakes
they're ensured uniqueness
green oriole
Yep you're right, it is written in the doc
I thought someone said me they can overlap
My bad
glass pecan
if you'd like to look at an eq implementation, give me a moment
clever wraith
None of us ever worked in discord anyways
glass pecan
I'm unsure your point AG
green oriole
Are you talking about the worker ID?
glass pecan
they didn't
clever wraith
how do you know
green oriole
Just by looking at the doc
glass pecan
because we work with it enough and get enough info through docs, updates, blogs
glass pecan
they used the snowflake ID model created from twitter because it can be sorted and retrieved semi-reliably by datetime (as the IDs are generated based on the current unix timestamp + relevant IDs appended to the end)
it's a good ID model specifically for nosql data storage, which they use. They rely on a db called Cassandra, which is designed to be most performant for quick and easy writes
in return it has lower performance with reading data
clever wraith
TBH we are making it hard for the user in the end
glass pecan
no we're not
clever wraith
its ok how it is now
glass pecan
the user experience has not changed
clever wraith
ok give me the syntax i will change it , test it , lint , push it
glass pecan
Are you asking for the code?
glass pecan
after the sig is changed, there's a small amount of adjustments still required in the function code too to use the message object rather than parsing the jump_url
clever wraith
i think i will take some resst
my mind can't process anymore
TBH
just let it be , it is
I am super tired
glass pecan
If you want to rest, that's fine. But you seem to be making this whole process a lot harder than it needs to be
clever wraith
cough cough
glass pecan
You've been questioning a lot of the review comments for no reason, you've marked unresolved comments as resolved, you've got a strange attitude anytime someone suggest a change
glass pecan
Which had at one point been marked resolved early. Unless a change has been made exactly as suggested, you should not resolve the conversation
But that's just one aspect of what I mentioned above
glass pecan
Yes, you just keep saying you want to keep it as is.
Reviews have asked for changes. This isn't for no reason.
clever wraith
cuz i don't really how to do that
i am not a experienced person
I never said i was
glass pecan
Then you'd ask how to do things, not just demand to keep it as is
We're a learning community, we are more than happy to step through how to implement suggestions when necessary
We don't expect everyone to know everything
But we do expect a willingness to be open and to learn
glass pecan
Like I said earlier, there's no rush and there's no need to prioritise this over anything else in your life. Just take things as they come.
At any rate, take some time to rest and come back when you've got some spare time. We can help go through any aspects necessary with you, there's no need to be overly frustrated trying to do it alone.
crude gyro
?
clever wraith
Should i add a feature that DM the person whose message is being bookmarked , just to keep him informed ?
@green oriole
green oriole
Don't think you need, that's pretty useless
Also, can you add some logging when a user create a new bookmark?
thorny wraith
Got some great contrib coming up....
green oriole
On which issue?
thorny wraith
Security...
green oriole
On.. Python discord’s projects?
thorny wraith
Nah, just a developer tool that I have built, free doe.
green oriole
This channel is for contributions for pydis’ projects :)
green oriole
Hey g!
clever wraith
@green oriole there ?
clever wraith
what do you mean by logging ?
green oriole
Doing logging : log.info(..)
I think just adding log.info(f"User {ctx.author.id} bookmarked message {message.id}" or something like this (you should test it first) should be enough
clever wraith
Ok
green oriole
Not sure of the syntax of the sentence, but yeah
clever wraith
Do you want to delete the ctx message cuz that contain personal detail like Hints how people name other people , how they remember and all
clever wraith
ctx message
.bm message
not from log
like I do .bm afasf
and that message get deleted
tawdry vapor
@crude gyro Is this still necessary? https://github.com/python-discord/bot/pull/76 If yes, should it be done for our API client too (it uses its own session)
glass pecan
i don't believe it's necessary anymore, at least last time i looked at it. but there wasn't a major reason to revert it either unless we worked on the session code again later down the line, so it's just been left alone.
glass pecan
nice
tawdry vapor
I kinda got stuck though
aiohttp doesn't want sessions created outside a running event loop, which is what both the bot and api client do. I can take care of that for the bot, but not for the api client. This is because the api client is needed for the api logging handler, which currently queues up log records until it detects that the event loop has finally started running.
Currently aiohttp doesn't enforce it (surprisingly I don't even see the warning being logged, not sure why). But it will in in 4.0, if discord.py ever upgrades to that
glass pecan
whres the api client atm
tawdry vapor
It's instantiated inside the bot
Well, before I subclasses it was created in __main__ and added as an attribute to the bot
I think if the creation of the API log handler is delayed, then the API client and its internal session can be created when the bot runs.
But this means log records before the bot starts will be missed
The handler does have to wait for the bot to start regardless, since it relies on discord.py to start the event loop.
glass pecan
they're just queing tasks, which is fine
ok, so
in APIClient
start with self.session = None
have a coro defined that creates the session and assigns it
tawdry vapor
If we want to also preserve those initial log messages I think we'd have to make it so the handler doesn't get the client right away (since it queues tasks, it doesn't need it until the loop is actually running anyway)
glass pecan
create a new attribute for APIClient that's an asyncio.Event
in init, do loop.create_task(self.setup_session())
the setup_session creates the session as well as sets the asyncio.Event
tawdry vapor
But it won't have a running event loop yet
glass pecan
doesn't need it
lets say the asyncio.Event is put on self.ready
APILoggingHandler.ship_off can start with the line await self.client.ready()
and everything else goes on as normal
this way you can keep queuing logs for the api, they don't actually try send until the api session has been created, and that will only happen when the loop starts up as its done within a task also
tawdry vapor
Ok that sounds promising
I'd need to raise an exception if the client tries to be used before it's ready though but that seems doable
glass pecan
the logging stuff
tawdry vapor
Ok sure but it's just a fail safe
we cant guarantee the next programmer will wait for the ready event
glass pecan
ok
then put the waits in the methods
that'll make it global and ensure all api session usages must wait until the http session is created
async def get(self, endpoint: str, *args, raise_for_status: bool = True, **kwargs) -> dict:
"""Site API GET."""
await self.ready()
async with self.session.get(self._url_for(endpoint), *args, **kwargs) as resp:
await self.maybe_raise_for_status(resp, raise_for_status)
return await resp.json()
Yeah I suppose that's better than an exception 🤔
glass pecan
won't even need to touch APILoggingHandler then too
since it's using it with .post
which will also have a wait
it's just to ensure the loop gets time to do the client creation first since there's no other wait to wait for that to happen
and the task to do just that will always be scheduled before practically any others
it's just asyncio has no guarentee for doing it in order so we gotta do this as a failsafe
tawdry vapor
Thanks, I will try it out
glass pecan
just don't forget to have the setup_session coro do self.ready.set() after creating and setting the session
otherwise nothing will go 😄
tawdry vapor
Yeah, and I realised it must set it instead of designing it so that it waits for the event
cause then the whole order of tasks thing comes into play
tawdry vapor
I may be overengineering this 
gusty sonnet
Can someone approve either the time unittest PR or the remaining time PR
So I can finally add test to the other PR lol
tawdry vapor
I've already done that right
tawdry vapor
they don't call me the review king for nothing 
They might get reviewed tomorrow since it's the weekend
Plus there are usually some reviews after the meeting
tawdry vapor
if only the earth were flat
what the hell is going on with these tests
they are nested extremely deep
maybe just a PyCharm bug
tawdry vapor
it reached the maximum recursion depth
I have a test failing for constants and I don't know why cause I didn't touch them
:\
oh it may be using my config
gusty sonnet
Lol, did you try running manually in terminal just to be sure?
gusty sonnet
Hmm, then yeah prob something with config
tawdry vapor
I just think my config has a mistake
I didnt expect the tests to use it
Yeah it's cause I am missing a wolfram API key
gusty sonnet
what the
tawdry vapor
it checks the type annotations of the constants
it's annotated as string
but missing results in a type of None
hence failure
glass pecan
hmmmmmmmmmmmmmmmmmmmmm
requesting my reviews
my review is:
Mark is a pretty nice guy and I'd buy him a beer if I met him in person.
4.5/5 stars
from bot.bot import Bot
lol
tawdry vapor
Hey man I told you the imports would be weird
glass pecan
ik, that's why i'm poking at it
tawdry vapor
I was thinking of importing it in init though
glass pecan
tawdry vapor
idk
glass pecan
in project init?
tawdry vapor
yeah
glass pecan
so it's just from bot import Bot
tawdry vapor
yea
glass pecan
yeah that's a possible thing but there's also not many things we import from bot atm
so
like i mean directly only from it
tawdry vapor
True
glass pecan
so yeah i doubt it's really going to be different
tawdry vapor
I personally don't mind either way
tawdry vapor
I had to fix the quotes in a docstring
I copied from website so they were the fancy quotes
glass pecan
fancy eh
can't be getting too fancy around here
wouldn't want to have people expect us to maintain such lofty expectations as smart quotes
tbh this looks pretty much exactly what i figured you'd do
the only thing i think needing to be done is to load it, make sure the bot launches and doesn't die a horrible horrible death
because pretty much any of this would cause instant errors if it was done wrong
tawdry vapor
Seems fine
I don't know how it'd go if it was to re-connect (dont think it would affect anything) or if it was to be stopped then restarted (which in practice we'd never do)
glass pecan
it should be fine
but there's an easy way to test
kill site while using bot
and then bring it back on
unless you meant discord reconnects, in which case they do nothing
tawdry vapor
Yeah I meant a discord reconnect
And by a restart I dont mean killing the process
I mean just stopping the bot but the process keeps running
like I said we'd never actually do that
But I made the effort to make it reset the sessions should that ever happen
glass pecan
how would you stop the bot but keep the process going. if we called bot.logout() it literally calls exit
tawdry vapor
does it exit?
glass pecan
we'd have to override that
tawdry vapor
i didnt know
glass pecan
yeah it does task cleanup then exit
inb4 i'm wrong and everyone points out how wrong i am
glass pecan
then where'd i get that stupid idea from
gusty sonnet
logout calls close
tawdry vapor
maybe once upon a time it did that
gusty sonnet
close is this
async def close(self):
"""|coro|
Closes the connection to Discord.
"""
if self._closed:
return
await self.http.close()
self._closed = True
for voice in self.voice_clients:
try:
await voice.disconnect()
except Exception:
# if an error happens during disconnects, disregard it.
pass
if self.ws is not None and self.ws.open:
await self.ws.close()
self._ready.clear()```basically killing stuff
glass pecan
thank you though
gusty sonnet
nvm me then, please continue
tawdry vapor
it's murder
glass pecan
it's definitely murder
but it's not suicide
so it seems it doesn't explicitly call exit
and maybe i was thinking of something totally different
gusty sonnet
murder sounds fun
tawdry vapor
So like I was saying, mainly not sure how it'd behave if it's closed then run is called again
glass pecan
oh you can call many clients at the same time
glass pecan
yes
glass pecan
yep
but you also can run multiple clients under the one token
but only if you want to have the lib scream at you for not being able to keep up with ratelimits
gusty sonnet
It shouldnt crash at all, hmm, discord does not forbid it either
Rate limit will be fun
glass pecan
however you cannot run the one Client instance in multiple threads
because it's bound to a loop
which is thread local
i'll try launching this then
tawdry vapor
just stash it
glass pecan
yes
stashing in amongst those, alongside heights, the deep sea and the feeling of being appreciated
all terribly frightning
gusty sonnet
Im terified by stashing
tawdry vapor
Just try it sometime it's not so bad 🙂
glass pecan
should have stretched
tawdry vapor
that sounds like a mess, yeh
glass pecan
that took like 3 hrs of messing around and like 15 minutes of actual coding
gusty sonnet
or else
tawdry vapor
pssh I still have things in my stash from a year ago
glass pecan
oh mark hasn't seen the proposal
gusty sonnet
Oh right
glass pecan
do you remember what the infr search output looks like mark
gusty sonnet
If you've seen it and if Scragly somehow lost them imma murder u
glass pecan
ye it is
tawdry vapor
with equal sign separators
tawdry vapor
kids have no shame these days 😔
hardy gorge
Anyway, scrags has made a nice version of it
tawdry vapor
how are those icons so tiny
gusty sonnet
If it has text mixed in they will be small
glass pecan
well yeah
gusty sonnet
Do fields!
glass pecan
aaaand that's a no lol
partially it appears to also be because of the emoji not being the full bounded size allowed
i blame @crude gyro
tawdry vapor
@gusty sonnet do you wanna review my pr too 
crude gyro
yeah that's probably my fault.
tawdry vapor
I could knock that out if you wanted. I can re-use the svg for the superstar icon I made
crude gyro
it's all fontawesome stuff
gusty sonnet
Time required to open Mark's newest PR in new tab: 5s
Time required to notice number of file changes: 0.5s
Time required to close the tab: 0.5s
Review was done in 6s. Or was it?
tawdry vapor
They're most just from fontawesome aren't they
glass pecan
ye
crude gyro
yes, with maybe like one or two exceptions
tawdry vapor
@gusty sonnet lol most of those are just import changes in all the cogs
glass pecan
if we're going to refresh the emojis, can we give them better names?
gusty sonnet
Yah I saw, I'll do a pull and test later, Imma find something to chew on first
crude gyro
I think I still got the icon PSD though.
tawdry vapor
They probably should be svg anyway, and I find svg much easier to work with actually - if I'm gonna be the one doing it
crude gyro
I hate svg so much.
crude gyro
but yeah you're probably right.
glass pecan
i'd be able to alter and make new ones easily with an svg around
tawdry vapor
I write the xml for the svg, I don't really use any program for it unless I need to do something that would be too complicated otherwise
glass pecan
that's pretty impressive
tawdry vapor
Well I am just gluing things together I'm not actually drawing complex paths in it or anything
glass pecan
i make mine in inkscape, simplify where i can and then run it through minsvg
tawdry vapor
my svgs at least come out clean
tawdry vapor
the ones from inkscape and especially illustrator have bunch of extra metadata and sometimes a nonsensical structure
glass pecan
it strips all that
crude gyro
sounds ridiculous to me to worry about xml cleanliness.
glass pecan
lol
tawdry vapor
Yeah probably cause you never have to edit the xml manually
but that's the way I work
molten bough
I have access to the font-awesome GH, since my FA was for PyDis anyway I can grab whatever you need
glass pecan
i love it when they're clean raw too, but also because it reduces the size a fair bit
tawdry vapor
Thanks gdude but I think all the svgs are available online, with inspect element if needed
molten bough
The premium ones aren't so easy to grab
tawdry vapor
I don't think we use the pro ones anyway
molten bough
actually I think it's time I sent someone the latest version probably
glass pecan
we do on the site in a couple of places. but in the icons, pretty sure they're just solid
molten bough
oh right, search is down isn't it
glass pecan
ye
tawdry vapor
So yeah I can re-do them in XML and if you want to re-name them then hand me a list of names
I mean SVG not XML
glass pecan
bot.api | WARNING | Cannot send logging record to the site: ClientOSError(104, 'Connection reset by peer')
feel like i've seen this before your edits too
wonder what the cause is
tawdry vapor
that error is pretty mysterious. I've had it for other things, not this though
molten bough
Latest FA is 5.11.2, latest DL I have is 5.9, so I can update you guys either way
not sure who I should actually be DMing them to though
molten bough
Alright
glass pecan
You've subcribed to daily FontAwesome updates! Reply with CANCEL to stop.
molten bough
haha
glass pecan
I'm sorry, I don't recognise that response.
glass pecan
not sure what that means
molten bough
tawdry vapor
Hey man someone will buy pro just for that one icon
oh
Ok I didn't know what you meant either then
tawdry vapor
@glass pecan Does the site log anything when you get that error?
molten bough
Nope, they made it free
glass pecan
nothing special, no mark
i changed the log to log.exception this time
but it's just showing what would be expected
tawdry vapor
maybe it's cause that session doesnt use the fancy connector
glass pecan
i thought you kept that in
tawdry vapor
I did
glass pecan
oooh right, only on http_session
tawdry vapor
but the api log handler has its own session
glass pecan
lets see
tawdry vapor
well, the apiclient has its own and the handler uses apiclient
glass pecan
im unable to do anything because my terminal has captured all input for some weird reason
dammit pycharm, wake up
ok
it happens no matter which is used
tried default and asyncresolver
both show connection reset by peer maybe 50% of the time
tawdry vapor
yeah I dunno
some specific way the stars align to cause that on your system
If it's 50% of the time then there's no way I could have missed that message
glass pecan
¯_(ツ)_/¯
well it's separate from this regardless
the logs are sending fine
and the api is working without issue
and like i said, this happened to me before the change
so yeah, everything looks dandy
tawdry vapor
So what is the verdict on the connector
keep it?
remove it?
use it everywhere?
if we remove it we get rid of a dependency
but async dns doesn't sound that bad
tawdry vapor
aiodns
tawdry vapor
you'll need to remove the connector then, to be clear
otherwise it will probably break cause the docs say it needs aiodns for that resolver
glass pecan
i removed any special things from both
just waiting the appropriate 52 years for pipenv to relock
i think it's stuck
it finally unstuck
wonder why it took so long. that's the longest it's ever been
tawdry vapor
usually when it gets stuck for that long it will just fail, so that's surprising
glass pecan
the removal made absolutely no perceivable difference
looks like you got what you wanted then; a chance to remove a dep
tawdry vapor
Well we do barely use http_session anywhere
majority of requests use different sessions
which would be the api client or discord.py's own session
glass pecan
that should actually be refactored
unless those sessions require a setup of default headers, it should be using http_session
glass pecan
other than the site api client
tawdry vapor
everything else would be using http_session, or should be
I don't recall anything creating a new session
Apart from discord.py of course
glass pecan
seems fine then
thought you meant extensions were randomly making new ClientSession instances
lol
tawdry vapor
My point was just that you probably didn't see any difference because most stuff doesnt use that session to begin with
glass pecan
fair
at any rate, i think it's likely pretty safe
you can ask lmn too if you want just in case
tawdry vapor
yeah, if we have two other sessions with much higher traffic that have been fine without that connector
glass pecan
yep
tawdry vapor
@crude gyro so should we get rid of of the AsyncResolver and AF_INET session stuff?
or the opposite, use it for all sessions?
clever wraith
@glass pecan randomly came out of nowhere and left a ton of thing to change LOL
hardy gorge
Since almost everything needs error handling, we have a central error handler that takes care of it. If custom behavior is necessary, a cog-level error handler can be used as well.
crude gyro
@tawdry vapor I think we put the async resolver in at some point to fix a memory leak issue
tawdry vapor
Oh dear
crude gyro
something about how we kept opening new sessions but not closing them
tawdry vapor
That's not observed anymore though is it
crude gyro
iirc @molten bough was the one who did it
molten bough
oh, yep, I remember
that was because of how asyncio handled its DNS resolver
it was basically spamming threads
tawdry vapor
Cause like I said yeah, if we have two other sessions with much higher traffic that have been fine without that connector
molten bough
if you have pycharm pro you can run the bot with the thread profiler
try it without my fixes, see if it still spams threads everywhere
tawdry vapor
I've never used that feature honestly but I will have a look tomorrow
molten bough
It's great, it can show asyncio coroutines as well
clever wraith
@green oriole removing the ability to select last message if none is given :C
clever wraith
Got it
clever wraith
This better pass now , i fulfilled Lemon, @green oriole , Scragly Request
meager storm
Any thoughts on me making a PR that adds html documentation via sphinx?
clever wraith
add a issue first
@hardy gorge
will this do if none is given ?
or this
Anyways DONE
used this one
long garnet
I'm having an issue setting up the bot for this second time, it's probably something I missed in the config file but no idea really
I don't know if this is the proper place to ask for help on that
glass pecan
sure
long garnet
but anyway, the issue I'm having is that !help doesn't work in any channels other than #bot-commands, I have an issue everywhere else
Ah didn't see you online!
here's the traceback:
damn Vim it doesn't work like Emacs
hold on
glass pecan
help isn't working in certain channels in the live bot either atm
pretty sure it's a bug
needs to be fixed
long garnet
well I do know where the problem lies
though it's pretty obvious
but I don't know anything of the code so
bot_1 | File "/bot/bot/cogs/help.py", line 453, in start
bot_1 | await session.prepare()
bot_1 | File "/bot/bot/cogs/help.py", line 207, in prepare
bot_1 | await self.build_pages()
bot_1 | File "/bot/bot/cogs/help.py", line 361, in build_pages
bot_1 | can_run = await command.can_run(self._ctx)
bot_1 | File "/usr/local/lib/python3.7/site-packages/discord/ext/commands/core.py", line 935, in can_run
bot_1 | return await discord.utils.async_all(predicate(ctx) for predicate in predicates)
bot_1 | File "/usr/local/lib/python3.7/site-packages/discord/utils.py", line 318, in async_all
bot_1 | for elem in gen:
bot_1 | File "/usr/local/lib/python3.7/site-packages/discord/ext/commands/core.py", line 935, in <genexpr>
bot_1 | return await discord.utils.async_all(predicate(ctx) for predicate in predicates)
bot_1 | File "/bot/bot/decorators.py", line 43, in predicate
bot_1 | if ctx.channel.id in channels or ctx.channel.id in hidden_channels:
bot_1 | TypeError: argument of type 'NoneType' is not iterable```and that's in this line
if ctx.channel.id in channels or ctx.channel.id in hidden_channels:
glass pecan
wonder which it is. channels or hidden_channels
long garnet
hidden_channels is None
long garnet
and I'm not an expert on linting
glass pecan
wheres that defined
long garnet
def in_channel(
*channels: int,
hidden_channels: Container[int] = None,
bypass_roles: Container[int] = None
) -> Callable:```but why is it None by default instead of []
long garnet
I see
glass pecan
because any modifications done to it carries across to multiple calls
since it's the same list object as was defined when the function itself was defined
so instead there should be a like that's like
hidden_channels = hidden_channels or []
within the function code itself
glass pecan
yes
if you do, let me know
this is considered an annoying bug and i'll review and merge any PR immediately
just realised you said issue so corrected my statement so you don't think i'm forcing you to code a fix
long garnet
yes but I did want to tackle it myself
long garnet
doesn't matter, I'll take something else
long garnet
yes
glass pecan
just do that edit and create a PR
long garnet
but the writing the issue will take me 6 hours and I'll make GitHub crash with my poor skills
glass pecan
lol
99% of the time, i'd say make an issue first
for this single line, don't bother
long garnet
okay!
glass pecan
sounds good
long garnet
I want my hand to be held
long garnet
seems to be working now
glass pecan
that is awesome news
long garnet
not sure if I should think of any other issues
long garnet
okay
glass pecan
but you've identified a bug
which is great
and the scope of the PR should be on that bug
long garnet
so, I'll show you the small tiny change too
glass pecan
we don't mind if you clean code up around the area you touched as a bonus, but if it doesn't need it, leave it 😄
long garnet
def in_channel(
*channels: int,
hidden_channels: Container[int] = None,
bypass_roles: Container[int] = None
) -> Callable:
"""
Checks that the message is in a whitelisted channel or optionally has a bypass role.
Hidden channels are channels which will not be displayed in the InChannelCheckFailure error
message.
"""
hidden_channels = hidden_channels or []
def predicate(ctx: Context) -> bool:
# ...```is there one for bypass_roles
feels weird to have it happen to hidden_channels and not bypass_roles
long garnet
no, that's why I mentioned before
long garnet
that maybe I should think of any other issues
that's what I meant
it doesn't happen now, but I guess it could?
glass pecan
fair. in this case, since both of them have the same structure, it's a potential issue if we don't pass it
so best to throw the same edit in for that arg
future thinking!
long garnet
got it
this is a good opportunity to check my broken English doesn't screw up the commit message
should I enter title + description?
glass pecan
i don't think i've ever seen you speak broken english
then again i don't follow you at every moment either
long garnet
I've seen myself doing so quite a few times
glass pecan
i reckon i speak bad english more and it's the only language i know
a concise title is required
is this PR or commit?
long garnet
do we follow that thing where we write them in infinitive?
it's just commit
but I'll write the same for the PR I guess
glass pecan
definitive language?
yeah it's best
a commit description is a super big bonus, yeah
long garnet
"Fix something" instead of "Fixed something"
glass pecan
it's not required though
yeah that's correct
also like
describe the code change
the description should have the reason
i've written an example, but i'll wait for you to show what you've got first
long garnet
I also have a question about commit messages
I should know my editor but I never write descriptions lol
glass pecan
sure
glass pecan
lol wahats going on there
also red means it's too long
here's the example i wrote up
Ensure hidden_channels and bypass_roles use a list when not passed.
The in_channel decorator raised 'NoneType' is not iterable when it wasn't passed, due to the default value being None but not checked against before iterating over it. This edit ensures the arguments are set to an empty list in cases where they have a value of None instead.
double enter makes a clear separation of title and description, yeah
long garnet
seems it doesn't count as the title if I do that
glass pecan
correct
long garnet
I was thinking of being way more descriptive than that, like "Given this line [quotes the line] ... then after [shows when it fails]..."
I think I'll use your example if that's okay
I'll try to read commit messages later to get the style
long garnet
no worries, have your breakfast man
glass pecan
Haha
Someone reminded me. Made me super hungry but now I guess I'll have to have a late dinner too
long garnet
I have not checked since I use another account in GitLab without much protection
how do I identify to GitHub from here? I assume it wants a token too
glass pecan
You need to have setup your git account with your cli git yeah
Give me a couple of minutes and I'll be back at the pc
long garnet
I would actually google it myself and read the docs but I forgot how it was called
2FA
I think I'll switch to SSH later
glass pecan
After you've enabled 2FA, you must create a personal access token to use as a password when authenticating to GitHub on the command line using HTTPS URLs.
ssh is better though, yeah
long garnet
$ git clone https://github.com/username/repo.git
Username: your_username
Password: your_token```oh no
glass pecan
excellent
long garnet
I was complaining that I need to write the token itself
(or paste it)
that's a bit annoying
glass pecan
lol
glass pecan
lol
long garnet
I lied, I added some small details to your description
glass pecan
its ok, i did a couple of small edits
long garnet
oh noes I forgot to codeblock some things
glass pecan
clodeblock lol
glass pecan
no probs, anytime
tawdry vapor
hmm
I don't know how to use this still @molten bough
does that mean it's spamming threads?
My vm was running low on memory so I had to kill it
I can't even see any console for the output of the process
and the bot never came online
Well using the connector everywhere doesn't seem to make a difference but that isn't exactly conclusive given I have no idea how to interpret the results
or if I am using the tool correctly
hardy gorge
What are you working on?
tawdry vapor
Determining if this connector is useful for the aiohttp sessions ```py
aiohttp.TCPConnector(
resolver=aiohttp.AsyncResolver(),
family=socket.AF_INET,
)
I was just reading back the conversation
I don't know anything about this, but that's a high thread count
tawdry vapor
Yeah but I suspect it may be it trying over and over again to do something
since the bot never even comes online
I have no idea what's going on cause I never get a console output
glass pecan
try running it normally
tawdry vapor
yeah it does run normally
glass pecan
yeah and do an eval that returns threading.active_count()
and do that a few times between playing around with commands
then compare counts with and without the special connector to see if there's much of a difference
tawdry vapor
5 threads vs three
that may just be cause I use the same connector instance for all sessions
I didn't manage to increase the thread count past 3 anyway
oh wait
it totally is going up without the connector
albeit gradually
Can you run an eval on production to see the current thread count?
hardy gorge
!int e
import threading
print(threading.active_count())
stable mountainBOT
In [1]: import threading
...: print(threading.active_count())
...:
32
Well that isn't too high but higher than a constant 3
My verdict is to keep it then
and use it for all sessions
molten bough
If you run it with the profiler (the green grid circle looking button) you can get a thread graph in one of the tabs at the bottom
It should give you an idea of what's going on
tawdry vapor
When I ran with the profiler the process just exits immediately
tawdry vapor
Yes. It will start if ran normally after all
molten bough
That's really odd
I guess it could be the VM, if you can't figure it out I'll see if I can do it when I get home after work today
tawdry vapor
You're welcome to try
Using evals I determined that the thread count is reduced with the connector though
So for now I decided that it should be used for all sessions
tawdry vapor
I don't know how constant it is, but trying to make the bot busy in roughly the same manner for both tests yielded a constant 3 for 1, and like 11-13 threads for the other
(it starts at 5 without it)
molten bough
I'm really not sure why asyncio does this specifically
Er, aiohttp
The solution came from someone else that had the same problem
It's not like they don't know about it
tawdry vapor
Beats me
I'm curious what the thread count will be after the bot has been running for a while
an hour after being deployed, it was 32 in prod
molten bough
It's based on how many sessions get created I think
One of the other possible solutions we had was a cog base class that came with a session
tawdry vapor
I don't think it's based on sessions; it's based on connections made aka the network traffic
Cause neither we nor discord.py recreate the session unless the the client is completely stopped or something
molten bough
Hm, okay
tawdry vapor
I guess any time it needs to have dns resolution it creates a new thread
molten bough
There is one thing I'm wondering though
It clearly doesn't happen with the aiohttp client discord.py uses, right?
I mean, if there are no cogs, does it happen?
Because you don't really hear people complaining about this
molten bough
I think someone was just wondering about it
But if the bug was fixed then it would've been obsolete code anyway
tawdry vapor
discord.py doesn't do anything special with its session
it too uses the default connector
but has provisions for specifying a different connector
I didn't do any tests for that though, only have it on all sessions
And yes, brought up out of curiosity since this code was old. Also, if we removed it we could get rid of the aiodns dependency.
clever wraith
Not sure if right channel but please add !codeblocks not just !codeblock, I make the typo like 3 times a day now
tawdry vapor
Yes there's a PR open for that already but it's blocked by some other stuff
green oriole
green oriole
I need to test it first
tawdry vapor
Yeah that's part of the reason why it's blocked
I actually don't know what's going on with the discussion on that so I will check
tawdry vapor
To be fair the discussion probably does not need to be private for this
But it's customary to create issues on there for the meeting agenda and the discussion just remained there I think
gusty sonnet
There are couple of ways to deal with it, it was a discussion about dealing with it client-side or server-side
iirc it was decided to be server-side
tawdry vapor
I feel like at this point we should just merge it and change it later, cause the work is already done
And nothing conclusive has come from discussion
so it's just in limbo
seems like a waste of functional code to me
gusty sonnet
rip my - https://github.com/python-discord/bot/pull/542 lol
I guess it's time for that new thing
The thing we acquired from the meeting
tawdry vapor
No I meant merge your PR
gusty sonnet
ping relentlessly
gusty sonnet
Pulling
tawdry vapor
green oriole
A little PR 
So now PyCharm shouldn’t complain about api_client not being an attribute of Bot right?
gusty sonnet
I'm so tempted to suggest py if ctx.channel.id in (channels + (hidden_channels or [])):and watch my suggestion roasted lololol
green oriole
Could be worse
gusty sonnet
Hmm, is there any reason you removed the log.info for the setup for wolfram cog @tawdry vapor
Nvm
More like it's the only one that has it
tawdry vapor
Oh then I just forgot
gusty sonnet
Yeah I just checked other cogs
None of them log anything
Prob a center log when loading
tawdry vapor
I did check for missed ones but apparently still missed one
gusty sonnet
Hmm
def setup(bot: Bot) -> None:
"""Load the BigBrother and TalentPool cogs."""
bot.add_cog(BigBrother(bot))
bot.add_cog(TalentPool(bot))```If loading the first fail, it wont load the 2nd right?
tawdry vapor
Maybe
gusty sonnet
Hmm, if any of the cog failed to load looks like the bot will crash instead
I guess an exception check there is not needed
bot (bot.bot.Bot):```Yes@clever wraith review done
gusty sonnet
@tawdry vapor would there be, in a blue moon, py await self.http_session.close()raise AttributeError coz of py self.http_session: Optional[aiohttp.ClientSession] = None
green oriole
The arguments
tawdry vapor
@gusty sonnet I believe no because the super method will return early if the discord client is already closed. But not sure if it's considered closed when it's instantiated or only when explicitly closed.
gusty sonnet
Hmm
tawdry vapor
Other than that if someone manually reassign it to None it would break too of course but this is on the assumption no one will do that
gusty sonnet
Yep, this is purely assumption that it is even possible for that
tawdry vapor
Same with prematurely closing the session
gusty sonnet
It is
hmm
Done
The beefy part is the bot and the api
The rest are just import changes
gusty sonnet
No, I will stick to the current logic, since difflib wont be any different than what fuzzy can do
The problem is whether we want this on the bot side
tawdry vapor
Or do you think it's not worth it if the code might be tentative anyway
tawdry vapor
Well wouldn't difflib be simpler code?
gusty sonnet
Create / delete will add to the cache directly
It might be simpler, but the results are not as conclusive
tawdry vapor
I mean if like 1 function call can replace the whole custom one you've defined then that'd be worth investigating but I don't know if that's the case
gusty sonnet
Hmm, worth a try, I'll try again
It will be hard to read for oneline but yeah it can be shorter
tawdry vapor
Anyway I'll look tomorrow like I said it should just be merged instead of waiting for a decision this long
I don't mean one line
Just a question of
gusty sonnet
If anything I want to merge 679 or 680 first
tawdry vapor
Are you re inventing the wheel
gusty sonnet
Then merge the other right after with missing tests
Fundamentally it's different in algorithm and purpose
I used it with an assumption that people will type the tag in a short-handed way like f-str / fstr / fstring
clever wraith
@green oriole u mentioned to use k and r 2 times but i can only find only 1 that should be k and r . the one with args
green oriole
embed.set_tumbnail doesn’t use it
@clever wraith
async def bookmark(self, ctx: commands.Context,
target_message: discord.Message, *,
title: str = None) -> None:```->
```py
async def bookmark(
self, ctx: commands.Context,
target_message: discord.Message,
*,
title: str = None
) -> None:```You might also want to move that * down the line probably
glass pecan
@clever wraith stop
you've now pushed two commits that failed to lint
you have clearly not installed the precommit we asked you to
do that first, because any and all lint errors will prevent a commit from actually happening if you had that installed
green oriole
pipenv run precommit
glass pecan
also make sure your commit title explains what you're changing, not why you're changing it.
this isn't descriptive enough
green oriole
Okay, he/she is committing from the web interface
Can you please use the command line for it
So you can run precommit before
molten bough
gusty sonnet
Hmm, inspecting got me https://www.mouser.com/images/suppliers/logos/adafruit.png
But it doesnt show on my chrome
molten bough
Check the console
clever wraith
its just a indent issue
gusty sonnet
Ah yes, a reload did the trick, weird, I did reload 5 times
clever wraith
cuz i am on phone
gusty sonnet
glass pecan
please don't edit your PR on your phone
green oriole
Well, do it later then
clever wraith
@green oriole you have to do it . i have no access to PC till 29 now
glass pecan
akarys is not personally responsible for your PR
if you're unable to complete it, comment saying so on the PR and an existing person can choose to finish it
clever wraith
can i put it on hold ?
green oriole
Just leave a comment
glass pecan
it's near finished and not something that's absolutely requiring a specific person to finish, so no i don't think it's necessary to hold
clever wraith
i will call a laptop temp for few hour . from dad office to complete it
green oriole
Please setup precommit on it
glass pecan
no it's fine. it's just one indent
there's no need to resetup a whole new dev environment on another system
we'll tidy that last point and it'll be done then
tough imp
i can take a look at polishing it if needed
at least ill learn how to add commits to someone elses pr
green oriole
You have the remote branch written at the bottom of the PR, you just push to it
tough imp
@clever wraith would you be ok with me adding the necessary changes to your PR so that we can merge it?
crude gyro
just do it
@tough imp
AG said he was gonna be gone for like 3 weeks, and we don't want a PR just sitting stale for that long
tough imp
just do it
just did it
crude gyro
nice
clever wraith
@tough imp thanks a lot .
please use hyperlink for testing not message id . cuz it was made for hyperlink
woeful thorn
The point of the Message converter is that it accepts both
tawdry vapor
@molten bough Think you could have a look at these? As I understand it's just you and lemon really involved in there projects https://github.com/python-discord/django-simple-bulma/pull/36 https://github.com/python-discord/django-crispy-bulma/pull/29
green oriole
Also, can you disable check_suite on simple and crispy bulma please?
crude gyro
@molten bough @tawdry vapor I responded to those now
I don't know what check_suite is, @green oriole, but maybe you can make that PR yourself, explaining the why and how in the PR text?
molten bough
check_suite is a webhook category I think