This should be ignored
#dev-contrib
green oriole
molten bough
Which project?
molten bough
Yes, that error code is indeed not ignored on master
but I do see a lot of files that aren't annotated that way
it's not ignored on the time branch, which has different linting rules too
sounds like it probably is intentional
cold moon
I think I try to clone this again. Let's see does this fix.
molten bough
pre-commit will lint any python file you modify
tough imp
yeah but the error codes for flake8-annotations have changed from TYP to ANN
cold moon
ok
molten bough
perhaps pipenv sync after a merge from upstream master then, yeah
cold moon
ok
cold moon
Is there any discussion before about adding starboard to SeasonalBot? Like when at least example 5 users react message with ⭐ this post this message automatically to #starboard channel? Also, about bookmarking, there should quick way to make bookmarks with reacting to message with ✉️ and this automatically send this message to user DM?
glass pecan
we won't add a starboard
cold moon
Why?
glass pecan
this has been discussed quite a few times in the past internally, and it's decided every time against it
tbh i'm not really willing to open the conversation about it again, just be aware it's an established decision
if you want to enhance bookmarks, feel free to check any existing issue tickets about the feature to see if it's been discussed already
it certainly seems to ring a bell
tough imp
there's a PR open with enhancements to the bm command: https://github.com/python-discord/seasonalbot/pull/349
I'm not sure how staff would feel about bookmarking by simply reacting with the emoji to something
you can always open an issue, it's easier to keep track of the conversation that way
crude gyro
I don't like the bookmarking by adding an emoji idea.
the feature would have poor discoverability and would probably be surprising in many cases
for example, say someone talks about sending letters
and someone reacts with ✉️
now 10 people click that emoji
just because they're piling on
and all of them are getting DMs for bookmarking stuff
cold moon
Yes. Especially new users, who don't know what this do.
crude gyro
feels unintuitive to me.
cold moon
Now for me too
But about this current open PR, this review solution is that easy. I see so much ways to solve this. I think I'll create review with suggestions.
cold moon
https://github.com/python-discord/seasonalbot/issues/331 Can I work on this due currently there is no activity?
eternal owl
I think you can
tawdry vapor
@mellow hare When committing suggested changes please write better commit messages cause GitHub's defaults suck
brazen charm
I usually just do it in pycharm while addressing others and then add a co author
Not the fastest but helps if I need to change it more or spot a mistake along the way
mellow hare
Will do, apologies
eternal owl
this is good to go
@gusty sonnet I am not implementing the tags pagination
check this: https://discordapp.com/channels/267624335836053506/635950537262759947/685966577455464499
gusty sonnet
That's ok, I believe ELA made a precommit for the tags length as well
cold moon
@clever wraith I made suggestions to your seasonalbot bm command to solve review.
clever wraith
Can someone tell me why my test failed
it works just fine on my local machine
only difference is that i use 3.8 instead 3.7.5
tough imp
you can look at the job that failed in azure
Check for merge conflicts................................................Passed
Check Toml...............................................................Passed
Check Yaml...............................................................Passed
Fix End of Files.........................................................Passed
Mixed line ending........................................................Failed
- hook id: mixed-line-ending
- exit code: 1
- files were modified by this hook
bot/seasons/evergreen/bookmark.py: fixed mixed line endings
Trim Trailing Whitespace.................................................Passed
check blanket noqa.......................................................Passed
Flake8...................................................................Passed
your local environment is likely out of sync (why do you use 3.8?)
cold moon
I don't understand this error.
brazen charm
You need the new precommit/lint which also includes changing line endings to LF everywhere
clever wraith
but it lint fine here
brazen charm
Your environment is out of date then
clever wraith
how can i update it ?
brazen charm
with you also having the wrong python which can bring unexpected behaviour, just recreate it
cold moon
molten bough
Mixed line endings means you're working in an editor that is using CRLF line endings, but the repo uses LF endings
not all editors convert the entire file
gusty sonnet
@eternal owl can you also change the line in 154 and 164 - https://github.com/python-discord/bot/pull/803/files#diff-99a87ea027fd1dc2df94a6e8fb213f7cL150
clever wraith
yeap
molten bough
yeah, that should be LF
but
it depends how you have git set up
in pycharm, that's telling you what the file has
pycharm won't mix line endings in a single file
that said, if you made the file, it might be using the wrong ending
you can always check another file
eternal owl
I don't see those lines @gusty sonnet
gusty sonnet
It's this part ```py
@tags_group.group(name='search', invoke_without_command=True)
async def search_tag_content(self, ctx: Context, *, keywords: str) -> None:
"""
Search inside tags' contents for tags. Allow searching for multiple keywords separated by comma.
Only search for tags that has ALL the keywords.
"""
matching_tags = await self._get_tags_via_content(all, keywords)
await self._send_matching_tags(ctx, keywords, matching_tags)
@search_tag_content.command(name='any')
async def search_tag_content_any_keyword(self, ctx: Context, *, keywords: Optional[str] = None) -> None:
"""
Search inside tags' contents for tags. Allow searching for multiple keywords separated by comma.
Search for tags that has ANY of the keywords.
"""
matching_tags = await self._get_tags_via_content(any, keywords or 'any')
await self._send_matching_tags(ctx, keywords, matching_tags)```
Since self._get_tags_via_content() is not async anymore, awaiting it will raise exceptions
eternal owl
fak, I should be more careful
gusty sonnet
@eternal owl right now there is a tiny but where !tag search any would raise a AttributeError which is not from your code, but can you include a fix it really quick, or do you want me to commit to your PR? It contains a fix in line 158py async def search_tag_content_any_keyword(self, ctx: Context, *, keywords: Optional[str] = None) -> None:changing topy async def search_tag_content_any_keyword(self, ctx: Context, *, keywords: Optional[str] = 'any') -> None:
Otherwise everything works, and I should be able to approve it
cold moon
https://github.com/python-discord/bot/issues/796 Is there any news about this? Sorry, just I have nothing to do...
eternal owl
I think it will be best if you do it as you understand the situation better, and can write a better commit message 😅 @gusty sonnet
gusty sonnet
Okay, I'll push a commit
tough imp
the checks on this PR have been stuck since yesterday (I pushed multiple times), what can I do about this? https://github.com/python-discord/seasonalbot/pull/329
tawdry vapor
Usually changing the target branch works but it didn't this time
You may need to ask someone with access to azure to manually trigger a build
tough imp
ok, it's not a tragedy at least for now
tawdry vapor
This usually also happens when there are conflicts, but there aren't any
Try merging anyway
tough imp
theres a conflict
tawdry vapor
Oh
tough imp
does that cause it?
tawdry vapor
Then that is exactly why it's stuck
cold moon
On what issue I should work? I don't have nothing to do...
green oriole
Just pick one 
cold moon
OK, I picked one easy... Solved with some minutes...
tranquil topaz
i made a comment on the issue @cold moon
if i get more insight onto it i can approve the PR
cold moon
@tranquil topaz I'm currently setting dev engine again up for screenshots. I have some automations that deleted already original tests.
woeful thorn
Did you mean to link another issue in the PR?
cold moon
I mean to show how is result after modification (before is in issue that is linked)
tranquil topaz
i did not understand the issue, and hence i did not understand the PR @woeful thorn, just asking for some clarification on what the issue is all about
thanks ks 😄
cold moon
URGH! This config file writing for bot is so annoying. This should be automated in future...
woeful thorn
glass pecan
constructive discussion is welcome @cold moon, but you should probably tone down the expressions of disgust/impatience. it's a bit rude to anyone who's worked on areas that you're commenting on and honestly doesn't add anything useful to a comment.
cold moon
Yeah, but looks like I have to set dev engine again up to my Linux computer. Using Mac + Docker... is not best.
woeful thorn
Why would it change?
Just send yourself the configuration
That's the whole point of docker, it's self-contained
cold moon
It's show starting up in 0.0.0.0:8000 but I marked web:8000 to config. I don't know is this reason why bot don't respond to changes. (channel changes etc.)
Bot is just not responding to events. Commands works, but events not.
green oriole
I don't know if we should keep the mod log channel update with the new help system
Does moving channels trigger a message?
woeful thorn
No
green oriole
But the channels will not be renamed when they're aren't used too?
woeful thorn
No, the channels don't get renamed
green oriole
Okay nice
green oriole
Ladies and gentlemen, can we have more info about this error? https://discordapp.com/channels/267624335836053506/267659945086812160/689202983472201736
tranquil topaz
thanks for the clearification @cold moon 😄
eternal owl
https://github.com/python-discord/bot/issues/662
I would like to give this a try and also the one in the site repo
cold moon
Is this normal that !zen command show line from position 0?
hardy gorge
That's the idea, yeah
You can now use regular Python list indices to get the lines
!zen -1
stable mountainBOT
cold moon
Can anyone assign me in https://github.com/python-discord/bot/issues/589 ?
cold moon
Thanks
signal willow
Does someone know any C++ discord server?
cold moon
@signal willow Not good channel. Please use off-topic channel.
signal willow
ok
cold moon
I have question: How can Python Discord bots so fast? All bots what is made with Python is so slow, but Python bot and SeasonalBot is so fast.
woeful thorn
Because "slow" is a relative term
cold moon
I mean, response time
woeful thorn
?
cold moon
All other bots with Python have pretty big response time, but all bots here response instantly.
woeful thorn
I don't think the first half of the sentence is true, and even if it was there are plenty of other factors to consider than just "python is slow"
We don't do anything special to make our bot any faster than any other bot made with the same library
mellow hare
Keep in mind that when people say "Python is slow", it's still doing it in fractions of a second. It may be a couple more fractions of a second than it would be in another language, bit it's still crazy fast
As ELA said, it's all relative
tough imp
I think the limiting factors will mostly be discord latency, network performance or the hardware that your bot is running on
if your bot is slow to respond, the programming language it's written in is unlikely to be the culprit
sharp timber
Agreed. Generally it's dependent on the strength and latency of it's internet connection, and whether there's pathological code involved. Discord.py is async, so it's still all one thread despite being able to switch between tasks it's given. If you lock yourself in a loop within a loop without awaiting something there's going to be delays
cold moon
But one thing: When 5k users use bot same time, will this affect response time?
molten bough
most of the larger bots are servicing a lot of servers at once
each shard is limited to 1000 servers, but it's still a lot of events to be processing
cold moon
Yeah
molten bough
that's not the case with the bots here
patent pivot
I wonder how many events we actually get
molten bough
I expect quite a lot of presence updates
patent pivot
let's find out
molten bough
Stats \o/
patent pivot
how many events?
stable mountainBOT
Events per minute: 1180.8
stable mountainBOT
Events per minute: 1098.7878787878788
molten bough
That's a fair amount, yeah
mellow hare
Wait, is that an actual command?
mellow hare
Makes sense
patent pivot
yeah it was some joe fuckery
stable mountainBOT
I can confirm that. Joe always makes me do things.
tough imp
if any(
target not in seasonal_dir
for target in ("banner.png", "bot_icon.png", "server_icons")
) and (
self.current_season is not SeasonBase
):
can I get away with this lol
i guess the more politically correct alternative would be
if (
self.current_season is not SeasonBase
and any(
target not in seasonal_dir
for target in ("banner.png", "bot_icon.png", "server_icons")
)
)
What about
if self.current_season is not SeasonBase and any(target not in seasonal_dir for target in targets):
where targets is that tuple
the first is kinda hard to read
tough imp
mm I think that's a bit too long, I'd probably still prefer to multi-line it
but assigning targets above may be a good idea
maybe i can assign the result of any(target not in seasonal_dir for target in targets)
and then one-line the if statement
molten bough
You can always store the comparison result in a variable
It's often more readable than shoving it all in a conditional
tough imp
yeah but maybe that's too boring
i'm aiming for just the right amount of sauce
ended up going with this for now at least
branding_incomplete = any(
target not in seasonal_dir
for target in ("banner.png", "bot_icon.png", "server_icons")
)
if branding_incomplete and self.current_season is not SeasonBase:
whole function is a bit dense so may have to re-think my approach anyway
crude gyro
that kind of thing would've been my suggestion, @tough imp
I think that's pretty much optimal readability for a complex conditional
that layer of abstraction - e.g. "all these conditions are basically checking if branding is incomplete" - makes the latter conditional so much more pleasant to read.
tough imp
fallback on evergreen now functional 
crude gyro
nice, dude
great work on this stuff
looking forward to reviewing the PR
make sure to poke me when it's ready
tough imp
yee will do
molten bough
so Discord supports server templates now
that would be awesome for setting up testing servers
wonder how they're made..
brazen charm
oh that would be nice, would be able to fork the repos and "fork" the server
brazen charm
someone was also working on setting up the server from config so that ought to make that job easier with only needing to grab the config values and set them
rocky bloom
i mean what you could do is create a template for this server for bot testing and then have a pipenv command and a specific python file that auto-updates the environment vars with the correct IDs?
crude gyro
apparently creation tools aren't released yet
@molten bough yeah, if they ever do get released. my impression is not that they will be.
molten bough
someone I was chatting with said there used to be a tool in experiments, but I'm not modding my client to check that out
hopefully we get it, I guess we might not
crude gyro
hopefully we do
although I think it was @green oriole who was working on a bot that could automate the whole test server thing anyway
so it might not be necessary.
molten bough
Oh yeah, I forgot about that
green oriole
Yep @crude gyro, I was working on a snapshot system, but I think it was mark that told me that you chatted about it in the staff channels, and it was apparently not the right solution, but I never heard back from you
Issue right here https://github.com/python-discord/bot/issues/818
cold moon
@clever wraith I made new suggestion to your Bookmark PR review that will fix this.
cold moon
Huh
clever wraith
Reinstalling arch
I will merge it after i test it
cold moon
Ye
cold moon
Quick question about https://paste.pythondiscord.com/gasirejopi.py :
Why is _fuzzy_search raise StopIteration on _target = next(_targets) (line 57)?
green oriole
Because it is an iterator reaching the end of its iteration
cold moon
But when I do same thing manually, everything work correct
cold moon
Oh, i know! It's mock without return value!
tough imp
raising StopIteration is how an iterator signals that it has finished yielding values
>>> it = iter([1, 2, 3])
>>> next(it)
1
>>> next(it)
2
>>> next(it)
3
>>> next(it)
Traceback (most recent call last):
File "<stdin>", line 1, in <module>
StopIteration
when you use a for loop, it listens for this signal and handles it internally
consider this:
>>> def gen():
... yield 1
... raise StopIteration
...
>>> for value in gen():
... print(value)
...
1
Just note that this will give you a RuntimeError in newer Python versions.
tough imp
returning in the generator will raise StopIteration, right?
Changed in version 3.7: Enable PEP 479 for all code by default: a StopIteration error raised in a generator is transformed into a RuntimeError.
didnt know this
green oriole
Yep
If you call it again using next(), you'll get a RuntimeError: generator raised StopIteration
Hey @hardy gorge, by the way, any news on this issue? https://github.com/python-discord/bot/issues/769
hardy gorge
There is some news, we did have a discussion about it during a staff meeting
I'll dig it up in a minute, there's another important conversation atm
green oriole
Sure
cold moon
Problem was this that I didn't set return_value to mock
hardy gorge
@green oriole
I'll give you a brief summary of the discussion that took place during the staff meeting. The idea is to create a #python-news (or similar) channel where we will relay news messages from comp.lang.python.announce and languages suggestions/ideas posted to python-ideas. We still want to announce version releases as well, in our regular announcements channel, but we're unsure of how best to best tackle it. It's usually the case that multiple versions get released on the same day and having four version announcements automatically posted seems a bit excessive.
As for the relaying itself, there were two suggestions: Signing up with a pydis email address and relaying it using mailgun (if possible) or using the mailing list API
@patent pivot was looking into this originally
patent pivot
Yeah I've been meaning to do something about it but I've been quite ill recently
Either way there is an API on the python mailing list
It's on the ticket we have internally so I'll go have a look now
This is what I found https://mail.python.org/archives/api/
green oriole
Okay, I think it will be way easier using the API
You aren't interested in following python-dev?
hardy gorge
We talked about it, but these ones are both the most interesting and have a manageable volume
green oriole
Okay, so is it a go, or not yet?
hardy gorge
The idea in general is a go, but the specific implementation still needs to be put on paper
cold moon
Have staff discussed about !package command?
hardy gorge
No, there's been no discussion about that yet, I think
green oriole
Nice, the API just 502 on bigger mailing lists
I guess we'll have to use mailgun somehow
green oriole
I did some research and here is I think a way to implement it, what do you think @hardy gorge https://github.com/python-discord/bot/issues/769#issuecomment-600714532
cold moon
Is this good idea to make such dynamic tests like https://paste.pythondiscord.com/haxixabaqe.py ?
green oriole
No, you shouldn't rely on a random factor for your tests
cold moon
But using real cache?
green oriole
How does the cache is populated?
green oriole
You should create it manually then, or you'll also test if the cache is correctly populated
cold moon
It's tested in get_tags, should I retest them?
green oriole
No no, that's a bad thing to test it here
You want your test to fail only if _get_cache doesn't work properly
cold moon
ok
hardy gorge
@green oriole I think mailgun allows you to relay incoming messages as a POST request to a webhook. It's probably not compatible with Discord by default, but we could have the site relay it.
That means we can just relay it based on the receiving of a message on a specific email address
green oriole
Based on what I saw, we'd need something in the middle to re-encode the request
So yeah
hardy gorge
Yes, but it shouldn't be very difficult
Mailgun allows you to set routes that make POST requests to an endpoint you set
We can create an intermediate phase on our site as that endpoint which will in turn send a request to a channel webhook on Discord
No need to get the bot involved for that
green oriole
Yeah, it will be way easier, I'm going to transfer the issue
patent pivot
@green oriole no, you did it wrong
the api does not 502
at least not when I last checked recently
green oriole
I mean
patent pivot
hmmmmmm
Okay
I don't like the mailgun solution
It sounds hacky and easy to break
I like this one better
hardy gorge
It's not an uncommon approach, though
patent pivot
It returns HTML, the first div there has an a with a href of /archives/list/python-announce-list@python.org/thread/PCZJYTHK6HQUVZMIGSLY3KUECJYWBUNQ/
we put that ID into the API and: https://mail.python.org/archives/api/list/python-announce-list@python.org/thread/PCZJYTHK6HQUVZMIGSLY3KUECJYWBUNQ/
bingo
hardy gorge
That means we'd have to poll the recent-threads stream, keep track of new threads, then do some more polling to get the messages to relay it right?
Why not just go with the mailing list-driven approach of relaying?
patent pivot
do we necessarily want every new message on the mailing list relayed?
woeful thorn
No
patent pivot
we just want the new threads
hardy gorge
Okay, then this is a better approach
patent pivot
so we just poll the recent-threads thing, parse the threads we haven't handled yet and throw the IDs into the API
it would be simple to beautiful soup it
I'll write this up on the issue @green oriole
I'm also going to transfer the issue back to bot
Okay @green oriole, new approach written up
Also in future, please refrain from transferring issues around
cold moon
How to call real function in test function that patch this function? https://paste.pythondiscord.com/wahixusufi.py
hardy gorge
That doesn't sound like something that you'd normally do. Can you explain your use case?
cold moon
I don't want recreate Tags._fuzzy_search, but this currently mock this and I want to get values from original for mock.
hardy gorge
I don't quite get it: If you're currently mocking it, you're in full control of the return values it will give, right? Why do you need the original?
And, if you're going to provide it with the original return values, why are you mocking it?
cold moon
I think then is better remove this mocking. this is too confusing
hardy gorge
I'm just not really understanding what you're trying to do, so it's difficult to help you
Normally you'd mock something so you don't have the deal with using the actual function you're mocking
You just provide the mock function with stand-in return values for it to hand out
As the actual return values don't matter
cold moon
I don't understand this myself too anymore. Better is remove it...
When you have time, I fixed !zen command PR
hardy gorge
I'll have a look later, but I think ELA had some things in mind
eternal owl
https://github.com/python-discord/site/issues/311
can I work on this?
and the issue related in the bot repo
cold moon
Anyone have ideas (and how) to add more assertion to https://paste.pythondiscord.com/wumaxohuwo.py ?
green oriole
You should take function calls one by one and assert called the right number of time plus the arguments, and the return value
That way, you shouldn't miss any
green oriole
You need to mock functions inside the cog
cold moon
Huh, this may go difficult...
Or wait, is this like @patch("bot.cogs.tags.Tags._get_tags_via_content") will this use this mock inside self.cog?
cold moon
It's confusing me
tawdry vapor
What exactly are you trying to do?
cold moon
I have self.cog as a instance of Tags cog, but I want to get like call_count, assert_called_once_with etc. of Tags._get_tags_via_content (and more)
green oriole
You should ideally mock self.cog._get_tags_via_content
cold moon
Oh, so @patch("self.cog._get_tags_via_content")?
tawdry vapor
No that won't work
Just make the attribute a mock
self.cog._get_tags_via_content = AsyncMock()
cold moon
OH! Now I understand. Thanks
tawdry vapor
The cog is in the setup fixture, which means it will get re-created for every test. Therefore it's OK to change attributes without worry of affecting other tests
cold moon
Okay, now I understand. Thanks
tawdry vapor
Just make sure the mock is created inside the test not in the setup fixture
green oriole
You should take a look at some tests of the snekbox cog, maybe it could help you a little bit
clever wraith
Another lint fail another tilt
ok i need to read this error , cuz it ain't happening here on my side
Can someone tell me why it failed
azure logs doesn't seem to help me
any of the admins please :)
https://github.com/python-discord/seasonalbot/pull/349/checks?check_run_id=519966310
woeful thorn
Mixed line ending........................................................Failed
- hook id: mixed-line-ending
- exit code: 1
- files were modified by this hook
bot/seasons/evergreen/bookmark.py: fixed mixed line endings
You should update your linting settings
tawdry vapor
Click on "View more details on Azure Pipelines" at the bottom of the page to see the full log
woeful thorn
And run them before you push
clever wraith
I do lint before pushing
cold moon
So I need to create mock inside test not in setUp?
tawdry vapor
@cold moon Yes, in the test.
cold moon
Maybe you use old linting @clever wraith
clever wraith
the git fork is quite old
tawdry vapor
@clever wraith That's good, the problem is just that your branch is out of date.
clever wraith
so update it to latest ?
clever wraith
what was the command
let me google it
tawdry vapor
git fetch origin
git merge origin/master
Oh it's a fork
so replace origin with upstream
clever wraith
in both command ?
fatal: 'upstream' does not appear to be a git repository
fatal: Could not read from remote repository.
Please make sure you have the correct access rights
and the repository exists.
Which repo is this?
clever wraith
cold moon
tawdry vapor
Do you use ssh for git?
clever wraith
tawdry vapor
If you use ssh, then git remote add upstream git@github.com:python-discord/seasonalbot.git
Otherwise git remote add upstream https://github.com/python-discord/seasonalbot.git
green oriole
The docs says to use typing.Mapping for arguments instead of typing.Dict 
woeful thorn
ok
clever wraith
ok now it passing lint in the new way
what i did
git clone https://github.com/python-discord/seasonalbot
linted it (pipenv run lint)
but i can't update cuz there is no changes
ok i added a . so i can commit
it still didn't lint
what is mixed line ending ?
cold moon
Do you use PyCharm?
clever wraith
not rn
I am using VSCODE
and i am using LF
but i commit a different way
I edit file in github website and paste the file content
mad lad
cold moon
@clever wraith Run pipenv run lint locally first. This will fix code. After this push.
clever wraith
I did that
cold moon
Hmm, this will fix that. I'll PR to your fork with fix.
clever wraith
Am i allowed to close this PR so that i can update my fork and commit it all at once ?
cold moon
What?
clever wraith
I wanna start with a new PR with my file at once
you can't fork multiple time
so i need to delete this fork to get a new one . rather then upstreaming it
cold moon
No you don't need.
Update master from upstream, then create new branch from master and it's up to date
clever wraith
I don't think i can change branch
cold moon
Huh, I think yes there is better to restart, due Attempt to fix lint issue and Reattempt to fix lint issue is not good commit messages.
crude gyro
<@&295488872404484098> <@&267630620367257601> <@&267629731250176001> <@&267628507062992896>
With help from @woeful thorn, I've written a song about the importance of linting your code before you push. Please give it a listen, and remember, all you have to do to solve this problem forever is to run pipenv run precommit, and you'll never be able to push an unlinted commit again.
hardy salmon
thank you
cold moon
Woah, nice
valid quest
Hah cool! thanks
hardy salmon
wow that got dark very quickly
cold moon
One thing I want to point out (I'm creating tests for tags.py):
@search_tag_content.command(name='any')
async def search_tag_content_any_keyword(self, ctx: Context, *, keywords: Optional[str] = 'any') -> None:
"""
Search inside tags' contents for tags. Allow searching for multiple keywords separated by comma.
Search for tags that has ANY of the keywords.
"""
matching_tags = self._get_tags_via_content(any, keywords or 'any')
await self._send_matching_tags(ctx, keywords, matching_tags)
keywords: Optional[str] = 'any' and matching_tags = self._get_tags_via_content(any, keywords or 'any') one of these is not needed.
crude gyro
that's not entirely true - the second one isn't completely redundant in this case.
if keywords was passed but was not truthy, for example of keywords is an empty string, then it would hit the second or 'any'
but since this is a discord command, I'm not sure that's possible without calling it directly from somewhere else in the code
which we really shouldn't be doing, anyway
glass pecan
it's not possible in this case only because this is a keyword only consume rest
crude gyro
right, that's true
glass pecan
if it was a positional consume rest or standard positional args, it would be with double quoting nothing ""
crude gyro
but still, if you called this function with keywords="", it would technically hit that part of the code.
is my point.
glass pecan
this is true, but like you said, we shouldn't be direct calling command callbacks so i'm not sure if that's too much of a concern
crude gyro
yeah it's not really a concern at all
so, ultimately you're right. the second or 'any' is redundant
although I guess you could argue it adds some readability since it makes the intention very clear
cold moon
How to test function that is inside function (!tags get)
light holly
cold moon
huh
gusty sonnet
Ah, I remember this
I added the commit to add that 'any' to the function because when I was testing it with PR 803, I realized that if I do !tag search any the keywords can be a None apparently, raising errors, so I added = 'any' to the function
The or 'any' is no longer needed, yep
clever wraith
https://soundcloud.com/lemonsaurusrex/lint-before-you-push
I feel attacked
cold moon
Can anyone help me with my question? Then I can continue work.
So I should remove this or 'any'?
gusty sonnet
You should if you modify the tag cog, otherwise it's not really breaking anything imo
What do you want to test inside !tag get
runic dune
lyrics are a bit more violent than i expected 😅
cold moon
@gusty sonnet https://github.com/python-discord/bot/blob/4a10284fb0d8373f225ec49739bed8d7e5d2a687/bot/cogs/tags.py#L170
gusty sonnet
You don't have to, treat it as a part of the get_command instead, and try to test get_command when it's on and off cooldown
cold moon
Ok
cold moon
One more thing I noticed: https://github.com/python-discord/bot/blob/master/bot/cogs/tags.py#L219 Tags is not longer in DB
green oriole
Hahaha, the lint before your push song is awesome
It is so silly haha
The song should be added to the contributing guidelines
hardy gorge
It's added to the pins here
gusty sonnet
You can argue that the resources folder is the DB now - it should serve as a warning still if the self._cache is empty
cold moon
Oh, quick question: how I should test paginated command?
File "/Users/user/PycharmProjects/bot-tags-tests/bot/pagination.py", line 203, in paginate
reaction, user = await ctx.bot.wait_for("reaction_add", timeout=timeout, check=event_check)
ValueError: not enough values to unpack (expected 2, got 0)
Got it. I just don't let it paginate
crude gyro
The song should be added to the contributing guidelines
@green oriole
I kinda like this idea.
not many contributor guidelines have music
glad you guys like it 
valid quest
Its awesome
lapis hull
This is great, well done 😂
sullen phoenix
cold moon
https://github.com/python-discord/bot/blob/master/bot/cogs/tags.py#L131 Why is this adding these tags there? On L134 it's overwriting it.
hardy gorge
The paginator will overwrite the description
This may be an artifact from a time before the paginator was used
cold moon
Should I remove this?
hardy gorge
There may be something I'm overlooking
Did you look at the git blame to see who added what?
gusty sonnet
I did, it was added later, and yes it is overwritten
I forgot to delete that, good catch
It was an artifact before paginator was added
mellow hare
The before time
Woah wait what? Since when can you batch commits from code reviews?
I thought they had to be handled one by one
brazen charm
Been there when I started contributing to the projects here iirc
mellow hare
Huh, fair enough. It says "New!" in its little description, but it's likely I just never noticed
Still, pretty cool
brazen charm
Could be remembering something different but when I spotted it a few days ago after a review I thought it was always there
cold moon
I removed this now and I'll write PR now (tags fix + tests for it)
Again. F. Why always locally tests work but in CI not...
I don't understand, why class and classmethod is switching in CI? It's working correctly locally
And !tag clas give it too in correct order... WTF is wrong with CI
I can't find what is wrong, due locally everything work.
crude gyro
@mellow hare hasn't been there since the start, but arrived at some point last year
mellow hare
Shows how much I pay attention. Awesome feature
cold moon
And I can't make any changes too due this will not let me commit due precommit
cold moon
When I switch them, I can't commit myself, due precommit fail locally. I think I must commit this directly in github. Just 2 items to switch.
tawdry vapor
Is it the mixed line endings issue?
woeful thorn
Committing it via GitHub isn’t the answer
mellow hare
Are you positive it's an issue with the test and not with the code? If it's not giving a consistent response between two different platforms, that points more towards the implementation not being consistent enough
cold moon
Huh, fixed
crude gyro
wanna share with the class how you fixed it?
cold moon
I needed to switch 2 tags positions
brazen charm
Is there a plan to enforce tests on the bot after it gets a high enough coverage?
woeful thorn
I don't know if we'd get to the point of requiring 100% coverage, but it might get to a point where we'd want to enforce no decline in coverage
It's a bit harder for bot than it is for the site since there's so much mocking that needs to be done to take the APIs out of the loop
It can add a lot of complexity that folks might not be used to when writing tests for other things, so it's a good learning experience but making it mandatory might turn folks off of contributing
cold moon
@woeful thorn So I should create constant CACHE and make there some foobar tags and overwrite loaded cache?
woeful thorn
Yes
You only need enough tags to check for proper functionality in the methods being tested, their contents don't matter
weary rampart
@clever wraith suggested a .xkcd command for SeasonalBot, and I'd like to try and implement it. It seems pretty inline with stuff like the Reddit, or the games/movies. Is the right approach to ask here w/details first, or to create an issue on git for it first?
tough imp
best to make an issue you can include details about how you'd like to implement it
i think it'd be a cool feature
in the template that github will offer you theres a tickbox that says "i'd like to implement this myself"
weary rampart
Thanks - I've submitted the issue
cold moon
Can I work with this: https://github.com/python-discord/site/issues/336 ? I think cleanup will be good first issue on site.
green oriole
What is the pydis dockerhub name?
woeful thorn
pythondiscord
green oriole
Thanks
tawdry vapor
Anyone have an idea why this doesn't allow empty values? https://github.com/python-discord/site/blob/master/pydis_site/apps/api/models/bot/message.py#L48
Is it an intentional design choice or an oversight?
We were relying on a bug that got fixed in DRF 3.10 https://github.com/encode/django-rest-framework/issues/6597
woeful thorn
Is this model specific to tags or is the validator just poorly named?
green oriole
I have a question about the site docker-compose : it is written that the migrations are automatically applied, but what is applying them? The manage.py run script?
tawdry vapor
manage.py is applying them
cold moon
How site give API key to bot in Docker?
tawdry vapor
Through an env var
brazen charm
you set the key for bot
cold moon
So site set env variable that bot read?
tawdry vapor
It's hard coded. You don't need to set a key when in debug mode (which is on by default) since it will use a constant API key during debug mode.
No, the bot sets an env that the site reads.
brazen charm
in bot's .env; don't believe I set anything for site
tawdry vapor
The api key is sent via headers when sending HTTP requests
cold moon
ok
green oriole
Thanks!
tough imp
I've ran into an annoying problem that is probably caused by my incompetence but I've been staring at it for so long that I'd like to ask someone take a look at it with me
it's a little bit difficult to isolate but the pattern is as follows:
I have a cog at bot.branding in which I define a custom exception:
class BrandingError(Exception): pass
A command in this cog will raise the BrandingError, so it gets wrapped in CommandInvokeError and passed to our error handler
within the error handler, I want to check that the wrapped exception is an instance of BrandingError so that I can handle it in a specific way
so the error that comes in is of type CommandInvokeError, and it carries an original attr which I expect to be of type BrandingError
which appears to be correct, but isinstance(error.original, BrandingError) always gives False
and I cannot for the life of me figure out why this is happening
log.info(f"Raised error type: {type(error)}")
log.info(f"Raised error type (original): {type(error.original)}")
log.info(f"Original isinstance of {BrandingError}: {isinstance(error.original, BrandingError)}")
Raised error type: <class 'discord.ext.commands.errors.CommandInvokeError'>
Raised error type (original): <class 'bot.branding.BrandingError'>
Original isinstance of <class 'bot.branding.BrandingError'>: False
the error handler of course imports the BrandingError from the branding cog
basically, it comes down to this:
log.info(BrandingError)
log.info(type(error.original))
log.info(BrandingError is type(error.original))
<class 'bot.branding.BrandingError'>
<class 'bot.branding.BrandingError'>
False
is the whole code anywhere on git or somewhere? That looks like it should work... maybe print repr of the original to take a look at it
tough imp
the repr of the original is just BrandingError('No such season exists')
repr of error is CommandInvokeError('Command raised an exception: BrandingError: No such season exists')
i could push it to a throwaway branch but idk if that would help since it's a bit of a mess atm I'll try to make a completely isolated example.. I'm just upset because this seems so ridiculous and I don't want to spend another hour on it lol
brazen charm
check the ids or idk if they're not the same object but should be... can't spot what's wrong above but could be missing the same thing as you are
Only thing that comes to mind is strings but don't think stuff should be returning them
tough imp
the ids are different, the mro is the same
log.info(id(BrandingError))
log.info(BrandingError.mro())
log.info(id(type(error.original)))
log.info(type(error.original).mro())
94655454375088
[<class 'bot.branding.BrandingError'>, <class 'Exception'>, <class 'BaseException'>, <class 'object'>]
94655455204576
[<class 'bot.branding.BrandingError'>, <class 'Exception'>, <class 'BaseException'>, <class 'object'>]
it's like the class exists twice...
brazen charm
Not that familiar with how dpy handles it and if it could be creating it somehow, but now I'd just try searching for declarations of that class in the whole project
brazen charm
it does work when I throw my own error inside
tough imp
ok i know why it happens
oh my god
ok so in my case the class declaration runs twice
once when the error handler imports it
and then again when d.py loads the extension
brazen charm
Sounds like that shouldn't happen if it goes through the normal import machinery
tough imp
it works when i load branding first, and then the error handler
i.e. it doesnt reload the module
ill have to look deeper into why this happens and whether it's my fault
anyway thanks Num for looking at this with me
brazen charm
hmm yeah looks like d.py ignores the import cache python has with how it loads them but a normal import after loading goes without executing it again
tough imp
i guess in order for us not to depend on import order then a logical thing would be to declare the exception in a module that d.py won't load as an extension
ok, that's a lesson learned
brazen charm
maybe worth making an issue on d.py for it, simple containmnet check to see if it's already loaded so it doesn't need to overwrite it
molten bough
Have we considered setting this up?
stumbled upon that while trying to find a way to allow a hook to run and modify files without failing the commit (an unsuccessful search unfortunately)
cold moon
Does we need !tag zen anymore? We have command now.
green oriole
It could be useful for fuzzy matching, like typing !zenn gets the tag
cold moon
But then this should call !zen
And also, there should tag source where is PyDis project links
woeful thorn
@molten bough by setting up do you mean provide a template & adding the instructions to the wiki?
molten bough
It would be a little more convenient for people that are working with more than one of the pydis repos, I was thinking
woeful thorn
So, yes?
woeful thorn
Is there anything else that would be useful in a template?
molten bough
hmm, hooks are the only thing I can think of right now, unless you wanted to make Git handle the file endings as well - could put that in there I think
I think you can put that in a .gitattributes
although I admit I'm not well-read on the structure of these templates
woeful thorn
Me neither, but a little repo would be easy to set up if it would be useful to have
molten bough
From what I understand, Git just copies everything from the template dir directly into .git in your cloned/init'd repo
hmm
Yeah, really just looks like info/exclude, info/attributes, config and hooks are worth touching
maybe not info/exclude, that could get confusing
cold moon
OH! I found one thing: When using ctx.send_help (SeasonalBot), this send bot's default help command. To fix this, there is required to pass this new help command to bot.help_command instead just adding command.
tawdry vapor
I think the PR on the bot fixes that
Should copy over the changes to seasonalbot once it gets merged
Speaking of which, can you address the review comments @jade tiger
cold moon
Okay
Is there anything that is priority to do in SeasonalBot or Python bot?
I need new task
green oriole
I mean, they are priority tags
cold moon
What?
cold moon
ok
patent pivot
cold moon
Huh, unit tests
patent pivot
man I remember when nix contributed to the bot and we left 110 review comments https://github.com/python-discord/bot/pull/15
molten bough
Pepperidge farm remembers
cold moon
LMAOO
molten bough
that was.. something
cold moon
I think I'll try this https://github.com/python-discord/bot/issues/582
BRUH, this autocodeblocking commit messages is trash
you may, descriptive enough? LMAO
patent pivot
at that point commit quality was less important since we squashed when we merged
cold moon
lmao
patent pivot
yeah I searched for this a while ago lol
cold moon
Should I wait until my current PRs get merged before asking assignment to new issue?
patent pivot
➜ for d in `ls`; do;
echo $d; cd $d;
git log | ack "fuck"
cd ..;
done
bot
0% chance this will fuck anything up.
* Fixed stupid idiot f-string moron factory fuckup
Critical fix: python parsing broken with previous merge. This should fix it, logical fuckup on my account.
branding
code-jam-1
code-jam-3---funny-ideas
fucking camelCase? wtf al sweigart
haha
patent pivot
let's see the biggest commiters to all of pydis
molten bough
you'll probably still see me in there because of the uikit stupidity
patent pivot
lol
brazen charm
definitely gdude and mark up there
patent pivot
haha yep
hm maybe I should go by email
okay here we go
954 Gareth Coles
824 Mark
610 Johannes Christ
511 Scragly
454 Lemon (Conmodo)
386 Ves Zappa
287 ELA
286 Joseph
267 Lemon (gmail)
141 Inver (corporate email)
138 ELA (github email)
84 Shirayuki
76 Momo
73 Rohan
73 Inver (Github email)
Is that number of commits? jeez
patent pivot
yep
patent pivot
breaking that down for you:
587 site
233 bot
38 code-jam-3---funny-ideas
32 pydis-bot-core
31 seasonalbot
14 code-jam-1
13 python-challenges
5 branding
0 snekbox
0 site.wiki
0 salt
lol, yeah, that looks about right
green oriole
Nix the german buddy that contributed to fedora and know a crazy amount of things about programming?
patent pivot
yes
green oriole
Hey, I made 54 commits to the bot, 38 to the site and 5 to snekbox I should be in the list :P
green oriole
I don't think I did
patent pivot
64 akarys42@users.noreply.github.com
69 matteobertucci2004@gmail.com
yeah you switched emails
and if I do it by name then akarys42 and matteo bertucci are still separate
for d in `ls`; do;
cd $d;
git log | ack "Author: " | cut -d \< -f1 | rev | cut -c 2- | rev | awk '{print tolower($0)}' | perl -p -e 's/[^[:ascii:]]//g'
cd ..;
done | sort | uniq -c | sort -hr
953 author: gareth coles
737 author: leon sandy
646 author: markkoz
633 author: johannes christ
506 author: scragly
261 author: sebastiaan zeeff
222 author: christopher baklid
220 author: sco1
192 author: s. co1
131 author: mark
108 author: joseph
107 author: sebastiaanz
91 author: joseph banks
90 author: jeremiah boby
84 author: shirayuki nekomata
76 author: numerlor
74 author: kwzrd
69 author: joebanks13
66 author: rohanradia
64 author: akarys42
it didn't like the ø in lemons name so he is now leon sandy
patent pivot
yeah, that's why it won't be able to link the two
patent pivot
because your github account has multiple emails
so if I commit from joseph@josephbanks.me or 20439493+jos-b@users.noreply.github.com or any other emails I have it just registers the commits as jos-b
that's why if you commit and don't have the email tied to an account it will still let you commit if you have auth but just won't display a github user with the commit
near laurel
In that case, it commits with the system username@host and system name.
patent pivot
yep
near laurel
Having an author in Git commits are required. GitHub is just online host of repositories. So the authentication allows to push the commit, whoever authored them. If the account is registered on GitHub, then GitHub links them and does stuff around them.
patent pivot
yep
near laurel
I think to really make commit yours, one should be using GPG Keys instead. 🙂
patent pivot
agreed on that, gpg is the way to go
molten bough
There are issues with that on windows but I make it work
tawdry vapor
0 issues for me
patent pivot
are there any notable open source projects requiring gpg signed commits?
molten bough
I had to completely nuke my entire GPG tooling and keystore etc recently
near laurel
@patent pivot Not that I know of.
molten bough
It made commits hang forever
near laurel
Signing commits is like ensuring that this new bug was written by you to solve the old bug.
And I can't simply go ahead and link your EMail ID with some random commits to take down on your reputation.
patent pivot
I wonder how many of our commits are signed
near laurel
I am using Ubuntu & Mac, so I have GPG configured in mine nevertheless. There is no harm in signing.
Well, I was just really participating here. I am fairly new in here and don't know what the initial discussion was around 😄
patent pivot
57% of commits to pydis bot are signed
checking site now
I think 60% of commits to site are signed
near laurel
Where did you get that stat? I don't know how to get that stat.
patent pivot
now checking seasonalbot, I expect this one will be lower
@near laurel git log --show-signature and ack for Signature made
➜ git log --show-signature | ack Signature made | wc -l
445
so 37% of seasonalbot commit are signed
that makes sense
green oriole
I wonder if signing commits actually do something in term of security
near laurel
@green oriole It doesn't "secure" the commit. It validates that the commit was actually done by you and you only. Just like you'd sign an EMail using your GPG Key.
green oriole
Yes, but are those signatures checked anywhere?
near laurel
@green oriole GitHub does add a visual batch. You can add verification in your pipeline as well, I guess. I haven't done so. If you want, you can certainly backtrack and validate the signs using public key.
I mean, GitHub won't say the commit isn't signed so its invalid.
green oriole
Hmm okay, I guess it still was a good idea to set it up
near laurel
I mean, like I said, there isn't any harm in doing so. 🙂
dry turret
Ofcourse you'll no longer be able to deny it was your code :#
near laurel
@green oriole And hey, there is one level of security but that's on system level. If your GPG key requires password, then you may need to put that password each time you commit.
molten bough
@near laurel Bear in mind you're not signing commits when you sign a commit
It's confusing, I know, but you're actually signing off on the repo state up to that point
So you'll want everyone using it or there's no point really
near laurel
😮 😮 Wow!! That is a myth buster for me. Also, now I'll need to dig in deeper.
green oriole
Yes, you sign the commit hash, which rely on various data, including the parent commit
That's why rebasing change the commit hash
near laurel
https://git-scm.com/book/en/v2/Git-Tools-Signing-Your-Work Hmm. This explains it
Not entirely, but yeah.
tawdry vapor
Anyone up for a quick review? https://github.com/python-discord/snekbox/pull/68
tawdry vapor
1 and 2 are references to links at the bottom
tough imp
ah right
tawdry vapor
One link is for the image and the other is for the link when you click on the image
tough imp
I'm trying to figure what the best way to write a decorator that wraps a cog's task would be - it has to wait for the bot to be ready, so it needs a reference to the bot instance in order to await its wait_until_ready - of course the wrapped method takes a self which probably has a bot attr, but there's no way to guarantee that and relying on it feels awful
the approach before feels fragile, the approach after has to import bot
so bot cannot import from decorators as that creates a circular dependency
maybe someone can think of something smarter than that
cold moon
I can't find any good issue to solve...
tawdry vapor
@tough imp If you create an abstract cog class with an abstract property for bot then you can guarantee it will exist.
And you could make the decorator a method of the abstract class
tough imp
that was my idea originally but it seemed overkill
ooh ok
that may be worth it
I can resolve the circular dependency, just importing the bot instance feels ugly
tawdry vapor
Why couldn't you just make the decorated functions be responsible for waiting until ready?
tough imp
I could but then that will get awaited every time the function body gets called
so that's ugly too
tawdry vapor
No I don't think so
tough imp
it's just that I'm struggling to find a solution where I won't think ew
tawdry vapor
In fact that is a good thing
Because the bot may disconnect
And then reconnect later
cold moon
About tic-tac-toe command: Should this do 1 game at once or multiple games in row? And should this handle draws with new game?
green oriole
I think you should let the user decide
patent pivot
I say we keep it simple
If it's a draw, it's a draw. When the game is over it has to be started again.
we don't really want multiple games going on in the same channel, similar to how some of the snakes commands can only have one game running at once
cold moon
So this should check is channel free and user is not currently in any game?
cold moon
Should TicTacToe board in embed or standard message?
brazen charm
take a look at how battleship creates the board now
autumn marten
Can you make it so that if you mention @dusky shore it says something like:
Hello! My name is SeasonalBot and I'm an amazing bot with over 1000 contributers. My prefix is
.If you need help, please type.helpFor more information, please visit https://git.pythondiscord.com/seasonalbot
glass pecan
we occasionally mention the bot without wanting a response for referencing and for showing
glass pecan
i wouldn't really want it to obnoxiously reply with some text anytime there's a mention unless we explicitly want it to through a command
autumn marten
I mean if the message is the bot being mentioned
Not if the message contains the bot being mentioned
glass pecan
yeah i still don't really like it, it's not clear that we're invoking it
pretty sure a mention is acceptable as a prefix also so you should be able to do "<@&518742000514891776> help" i think
otherwise people can just ask
autumn marten
Some staff get mad at me for asking
glass pecan
hmm, i doubt that would be the case here unless you're asking in the wrong places making it offtopic or disruptive
that's the only thing i could think of for someone suggesting to ask in another place
autumn marten
I'm in DMs
glass pecan
you should ask questions in the server
autumn marten
I can't speak in most channels for some reason
glass pecan
can't how?
we don't have channel-specific limits
once you're verified, you should have access to all the channels you see other than log channels or announcements
autumn marten
Like it says something like this
glass pecan
what channel is that
autumn marten
glass pecan
shouldn't be limited in there. if you can see it, you should be able to write in it
cold moon
Should tic tac toe have cancel button (reaction) too?
green oriole
Hey @tough imp, just out of curiosity, are they actual symlinks, or in the way you’ll process them?
tough imp
they're linux symlinks, I'm not sure how they'll behave on windows
glass pecan
(specifically, there's an option to enable symlink support in git for windows setup)
or i think git config --global core.symlinks true in git cli
however this is still not optimal and symlink support in windows is not complete
so it's usually recommended to just entirely avoid symlink usage in git repos (unless your project is os-specific anyhow)
tough imp
it makes a whole lot of sense for this particular case but it'll require some changes in the way i resolve the download urls in the branding manager
or would you recommend we avoid using symlinks, Scrags? i thought it was a really good idea
but if it'll cause problems for windows users then it's not a good idea
glass pecan
whats the situation? i haven't been able to keep up with repo tickets and prs lately
tough imp
the PR description isn't too long and gives full context https://github.com/python-discord/branding/pull/46
glass pecan
ok
tough imp
i also had the idea that there could simply be a json which would provide the full paths, that would make it easier on seasonalbot's side but lemon didnt like it too much as it adds maintenance debt
which is true
(the json would be directly in the branding repo)
tawdry vapor
I think it wouldn't be the end of the world if the logos were either duplicated or the misc folder was just missing one size. But it's not ideal. Neither are symlinks apparently...
glass pecan
why do we need a misc directory?
tawdry vapor
For all the images seasonalbot won't use
tough imp
we usually have a 128, 256, 512 resolution versions of the branding, and other assets that should be bundled together
glass pecan
the only thing seasonalbot may potentially care about is that the path is correct for assets and that the file name is correct
so any other files can be organised however
for example, if there was only this example
├── branding/
│ ├── seasonal/
│ │ ├── christmas/
│ │ │ ├── server_icons/
│ │ │ │ ├── snowing.gif
│ │ │ │ ├── bells.gif
│ │ │ │ ├── santa.png
│ │ │ ├── misc/
│ │ │ │ ├── banner_2018.png
│ │ │ │ ├── santa_256.png
│ │ │ │ ├── santa_512.png
│ │ │ ├── banner.png
│ │ │ ├── avatar.png
then id just do away with misc entirely and put the files in the parent directory
if there's too many files in the seasonal directory, then organise them
tough imp
theres also like, 2018, 2019, 2020 branding that really should be put in separate dirs though
we dont want to throw that away, and ideally it should still be bundled with the season though
glass pecan
i don't follow
tawdry vapor
I prefer the misc folder, it's more organised. I like that the seasonalbot and misc stuff is clearly separated.
tough imp
for christmas atm we have misc/2018/..., misc/2019/... sub-dirs etc
glass pecan
so why can't it be /2018, /2019
glass pecan
because misc is mostly useless as a category name
feels unnecessary
i'm not saying to remove all sub directories
i'm saying use them with appropriate category names
if you want multiple banners, group them in a banner directory
tough imp
what would you call a subdirectory that just has different resolutions?
glass pecan
different resolutions of what? server icons? then i'd place it under the server icons directory
/christmas/server_icons/alt_sizes/santa_256.png or similar
tough imp
right, i see
glass pecan
the bot can focus on discovery of files in server_icons without delving into subdirs
so it won't impact the bot
tough imp
i dont know, i guess i just like the idea of grouping everything under misc because it keeps the root clean
glass pecan
one or two basic files in cases a subdir doesn't make sense in the seasonal folder wouldn't be an inconvenience or messy
tough imp
the bot already knows how to resolve the paths and won't break if there are more things, it just picks up banner.png, avatar.png and everything from server_icons if present
glass pecan
and in most cases, you'd likely have assets that have multiple version so will end up being placed in a subdir also so it wouldn't impact the cleanliness of root really, instead keeping things organised as necessary
tough imp
I think it's cleaner from the perspective that misc is strictly a folder that seasonalbot ignores, and server_icons is strictly a folder for seasonalbot
glass pecan
branding is not just for the bot though
it's for our members, for contributors and for staff
anything that's not the exact files it's looking for is "ignored"
so i see no reason to cater to the bot in that regard
and making a folder called "misc" that's really a folder that's meant to be "seasonalbot_pls_ignore" just feels kinda unnecessary
plus it means you don't really need some logical duplication (or symlinks) for humans/bots to coexist organisationally
tough imp
I don't really see it as something that caters to the bot - quite the opposite actually, as a real human bean navigating the repo is easier for me when there's a clear distinction between what the bot uses, and what it doesn't
but I don't disagree with you entirely, it does create an extra level of depth that doesn't have to be there
I just went with what we agreed with lemon on originally because no one else really said anything
regardless it's a minor thing
we can get rid of misc without any changes to the bot's current setup
but I'd rather wait to see if anyone else has any opinions on this
to be honest I do find the branding repo a little bit difficult to find things in, but I don't really know what to do about it
glass pecan
sure, i'm just giving my opinion in the end so it's fine to see how things go. I just have a dislike for un-informative folders for things that are meant for organisation. if it's not intuitive why something is organised a certain way, it can result in extra questions and confusion from people, and it adds to the effort of trying to help out, as minor as the additional effort is.
i was away for a bit there as i wanted to do a before/after comparison on how i'd adjust things for one directory so it's clear just in case too
currently /branding/seasonal/christmas has the following structure:
├── branding/
│ ├── seasonal/
│ │ ├── christmas/
│ │ │ ├── misc/
│ │ │ │ ├── 2018/
│ │ │ │ │ ├── festive.min.svg
│ │ │ │ │ ├── festive.png
│ │ │ │ │ ├── festive.svg
│ │ │ │ │ ├── festive_256.png
│ │ │ │ │ ├── festive_512.png
│ │ │ │ │ ├── festive_64.png
│ │ │ │ │ ├── festive_large.png
│ │ │ │ │ ├── festive_transparent.png
│ │ │ │ ├── 2019/
│ │ │ │ │ ├── banner.png
│ │ │ │ │ ├── festive_256.gif
│ │ │ │ │ ├── festive_512.gif
│ │ │ │ │ ├── festive_64.gif
│ │ │ │ │ ├── sticker.png
│ │ │ │ │ ├── tshirt_template.png
│ │ │ ├── server_icons/
│ │ │ │ ├── festive_icon.lnk
│ │ │ ├── banner.lnk
│ │ │ ├── avatar.lnk
and i'd instead be suggesting something like
├── branding/
│ ├── seasonal/
│ │ ├── christmas/
│ │ │ ├── merch_assets/
│ │ │ │ │ ├── sticker.png
│ │ │ │ │ ├── tshirt_template.png
│ │ │ ├── server_icons/
│ │ │ │ ├── 2018/
│ │ │ │ │ ├── festive.min.svg
│ │ │ │ │ ├── festive.svg
│ │ │ │ │ ├── festive_1024.png
│ │ │ │ │ ├── festive_256.png
│ │ │ │ │ ├── festive_3000.png
│ │ │ │ │ ├── festive_512.png
│ │ │ │ │ ├── festive_64.png
│ │ │ │ │ ├── festive_transparent.png
│ │ │ │ ├── alt_sizes/
│ │ │ │ │ ├── festive_512.gif
│ │ │ │ │ ├── festive_64.gif
│ │ │ │ ├── festive_256.gif
│ │ │ ├── banner.png
│ │ │ ├── avatar.png
hmm, ok
so in this proposed scheme the bot will cycle files from server_icons/, but ignore the subdirectories
glass pecan
yep
i have not actually looked at your deseasonify PR either, so apologies if I made any assumptions to your discovery process
tough imp
it'd still work fine, I just have to make it ignore directories in server_icons/ since now it just expects everything to be files
but that'd be trivial
glass pecan
anywho, that's just my thoughts, which are terribly late to the party regardless, sorry about that
tough imp
no thats fine, im genuinely happy for any feedback or opinions
maybe you could paste the proposed schema into a comment on the PR
so we can refer to it
glass pecan
sorry only just seen this, was in between responding to stuff and moderating
crude gyro
@tough imp in my original proposal, what @glass pecan is saying was basically also my intention
the misc folder was just an example
my point was that seasonalbot would look specifically for the icons and the banner and whatever else we want it to manage, in a specific location which would be the same everywhere
but that beyond that, everything else in there would be ignored
so we could create whatever folder structure we needed in order to best organize stuff in there
I also don't like the symlinks idea
I'd rather we just have duplicates, frankly
it's not like we're duplicating anything expensive
and I do like @glass pecan's idea of ignoring subdirectories even in the server_icons folder
that sounds convenient
tough imp
ok
green oriole
Just out of curiosity, what was that? https://discordapp.com/channels/267624335836053506/267624335836053506/692792548971774033
mellow hare
Not sure.
woeful thorn
!free someone help xd
mellow hare
Ah right. For some reason I thought it was for a !user command
I though the converter handles and tosses out anything that would throw an error?
On optional arguments I mean
woeful thorn
I don't know off the top of my head, what does the code say?
mellow hare
I'll go double check
Oh it's not marked as optional
Interesting
I mean it has a default but it's not like
Typed
I don't know if I'm making sense
Yeah, this is what I was thinking of
But it's not in the code, so I must have been mistaken. The only place that stands out to me where we use it is in the echo command
Well if nothing else I feel better that I'm not crazy and it is indeed a thing
tawdry vapor
Anyone up for going through these stale issues? https://github.com/python-discord/bot/issues?q=is%3Aopen+is%3Aissue+label%3A"status%3A+stale"
gusty sonnet
I will in a bit
eternal owl
I will have a look too
eternal owl
https://github.com/python-discord/bot/issues/557
is this still valid
tawdry vapor
I don't know why it was transferred from seasonalbot
I don't think the site's functionality of linking a GH account is necessarily useful because those users did not specify that they want to make the association publicly available.
I should comment that on the issue
cold moon
Huh, I have 5 PRs opened in bot repo...
cold moon
This tic tac toe is pretty hard to do. I think I first make player vs player, and then maybe later AI
eternal owl
AI wouldn't be too hard, need to do some searching
If a row/col has only 1 X or O, then add another one
Maybe hardcode some tricks/traps, just hardcoding starting positions
rocky bloom
my AI for Connect Four was pretty dumb tbh and it really doesn't need to be overcomplicated
it checks every possible place and sees what would happen if it puts it there vs if it doesn't
it checks to see if it can block the player if they already have 3 in a row and lining up a fourth
and it checks to see if the AI has 3 in a row and can win with the next counter
if neither of those applies, it fills in a random place
i mean
this is a discord server for python help, not a games server
so idt it needs to be complicated at all
cold moon
I searched and found something Minimax. I think I'll try this.
cold moon
I tested Halloween season and found such thing? Bug?
brazen charm
There is an issue for that iirc, or some form of storage
cold moon
It's storage, but is this sided with this?
cold moon
@tawdry vapor sorry about these language problems. English is just not my main language.
tawdry vapor
It's OK, I completely understand.
cold moon
About https://github.com/python-discord/bot/issues/662 , how should this add new nicknames with command? JSON file need to updated directly in repo due deployment will overwrite?
green oriole
@cold moon if you have some trouble with English inside your code (like me) you should download the grazie extension for PyCharm, it is the JetBrains' super orthograph corrector, it saved me a fair amount of time already
cold moon
OK, thanks
clever wraith
what does pipenv lock even does ?
I know that it verify hashes in Pipfile and Pipfile.lock
but is this necessary if there is no changes regarding Pipfile ?
hardy gorge
It creates a lockfile from the Pipfile by resolving the dependency graph
It should only be necessary if changes were made to the Pipfile
Normally, you'd pipenv sync --dev to create an environment from an existing lockfile
clever wraith
now this made me happy after a long time 😄
cold moon
https://github.com/python-discord/bot/issues/745 Is there any news about this? This looks like good thing to have: Tag requester can react with trashcan and this will delete command + tag response.
tawdry vapor
Sure
You can work on it if you'd like
Just leave a comment if you're gonna work on it
clever wraith
@tawdry vapor I DM'd lemon earlier this week about maybe extending snekbox to allow helpers to run Unix shell commands in #unix, which would allow concepts to be explained in a much easier-to-understand manner than what's currently possible - I'd like to hear your thoughts on this.
I'm currently trying to come up with a sufficiently secure design for doing so. It seems to me that the current NsJail configuration needs to be made less strict for it to be possible to simulate a more full-fledged Linux environment, which is why I'm not comfortable with letting regular users run commands. I think I'd like to use docker to provide a minimal container (bash + coreutils, don't think more is necessary) separate from snekbox's container, but I'm not sure how I'd go about implementing that with NsJail thrown into the mix as well, not to even speak about actually being able to control docker from within snekbox's container.
Perhaps an easier solution would be to mount a "unixbox" rootfs inside snekbox's container read-only, and utilise snekbox's NsJail installation (with a separate config with more loose restrictions) to simply create a chroot jail with that rootfs each time a command is run, but I'm not sure what the best way to keep the rootfs up-to-date in an automated manner is. Perhaps keeping a separate container on the host's docker install, and mounting that as the rootfs? I'm not sure how doable that is and what the implications for it are, but it would make it easy to make sure the system is up-to-date.
I would love to hear your thoughts about this all
tawdry vapor
Sure, I'll think about this and get back to you
clever wraith
Cheers
tawdry vapor
Do you think users could interfere with each other since it's all running in the same environment?
Maybe not a concern if its simply bash + coreutils and still read only
I don't know if it's feasible to run docker within docker - never researched or tried it, but doesn't sound like a good idea
glass pecan
it's not recommended, no
tawdry vapor
Why is keeping it up to date a concern? Is creating a PR to update things in the dockerfile not adequate?
molten bough
Running docker from within another container isn't hard, you can mount the same sockets, but it might be a bad idea if users can run arbitrary commands
tawdry vapor
Can you elaborate on your unixbox idea? I'm not sure what that would look like honestly.
Like is it separate binaries in its own directory, symlinks to existing binaries, something else?
I'm not well versed in linux/unix stuff
How do things like pipes work?
Do they need procfs mounted and be r/w?
clever wraith
Do you think users could interfere with each other since it's all running in the same environment?
It should not be an issue in a read-only environment
I don't know if it's feasible to run docker within docker - never researched or tried it, but doesn't sound like a good idea
I have looked into it. It's not necessarily hard or a bad idea, but requires a few things to be set up properly. I should note here that there are two ways of running docker commands within a docker container.
One of them is bind-mounting the docker socket inside the container. This is a bad idea and I want to avoid doing this if possible. It theoretically allows for running commands on the host machine from inside the container. Due to us using NsJail, this ideally shouldn't be an issue, but I would still like to avoid it in a container where we are running untrusted input.
The second method is dind - docker in docker. This is literally just running a separate docker instance inside a container (check out https://hub.docker.com/_/docker/). Like I said before, this has its own set of prerequisites, for example the parent container needs to be run with--privileged, but we do that anyways due to NsJail needing it.
Why is keeping it up to date a concern? Is creating a PR to update things in the dockerfile not adequate?
It's not strictly a necessary requirement or primary concern, but I'd like to build this with future-proofing in mind, so I'd like to make it relatively easy to keep up to date in an automated manner.
Can you elaborate on your unixbox idea? I'm not sure what that would look like honestly.
Like is it separate binaries in its own directory, symlinks to existing binaries, something else?
I was thinking of a regular root filesystem mounted in a directory which is then chrooted into. The most minimal implementation of this is downloading a filesystem tarball, extracting it, and runningchroot.
How do things like pipes work?
Do they need procfs mounted and be r/w?
No, pipes and stuff will work fine even with 0 mounts.
tawdry vapor
I like the unixbox idea just for the sake of simplicity
I think nsjail will be good enough for securing it
green oriole
I was working on something similar to that a few weeks ago 
green oriole
I created ext4 formatted file as loopback devices for basically every system, and mounted them as the filesystem root when I booted up an instance, but the requirements weren’t exactly the same, I wanted to be able to store the filesystem and be able to reboot it later
cold moon
Can I work with https://github.com/python-discord/site/issues/309 ?
green oriole
You should leave a comment on the issue too
clever wraith
loopback devices, while nifty, aren't too useful here, as we're not working with filesystem restrictions (think ext4 inside fat32) or need the filesystem to be writable - in fact we need the opposite
green oriole
Yeah, the requirements aren’t the same
I’m curious about why you think we should setup docker inside docker, isn’t NsJail enough protection already?
clever wraith
not for protection, but for easier deployment and management of the rootfs
clever wraith
no, that + nsjail should be fine
we don't need any extra mounts (like sysfs, procfs, etc) inside the unixbox either
however since nsjail allows custom procfs and tmpfs mounts inside chroots (isolated from the host), it would make sense to use them, as we can then use the unix commands to demonstrate stuff like ps as well
green oriole
The 283 files changed, 64 insertions, 0 deletions is pretty funny
tranquil topaz
@brazen charm are you there?
brazen charm
Am now @tranquil topaz
tranquil topaz
the hush command is active now 😄
tranquil topaz
you did an excellent job making it. It was a pleasure to review 😄
clever wraith
not exactly sure what is happening here
never mind, i'm dumb
missed a config value
@tawdry vapor do you know if there's any reason we're passing config options to nsjail in both the config file and as runtime flags?
brazen charm
Happy to hear that eivl
clever wraith
@brazen charm can you answer a question i asked in #community-meta since you are the author of it
cold moon
I now found solution: https://paste.pythondiscord.com/abuweqitiv.py but, someone who have better experience with Django have any ideas how to simplify this? (It's Off-topic viewset).
Ingnore printing. It's for debugging...
weary rampart
Am I missing some aspect of seasonalbots setup? I thought I had it all set up, most commands work, but anything that uses a http client session throws a weird error. I get the feeling its because I'm on Windows, but I can't find anything about it anywhere. Sorry if I'm being dumb, but I have no idea what the issue is. Here's an example of me trying to use .movies comedy
03/30/20 15:14:53 - bot.decorators DEBUG: Charlie#4186 tried to call the 'movies' command and the command was used in a whitelisted channel.
From cffi callback <function _sock_state_cb at 0x0378F538>:
Traceback (most recent call last):
File "C:\Users\Charlie\.virtualenvs\seasonalbot-hJOA8vcr\lib\site-packages\pycares\__init__.py", line 91, in _sock_state_cb
sock_state_cb(socket_fd, readable, writable)
File "C:\Users\Charlie\.virtualenvs\seasonalbot-hJOA8vcr\lib\site-packages\aiodns\__init__.py", line 104, in _sock_state_cb
self.loop.add_reader(fd, self._handle_event, fd, READ)
File "c:\users\charlie\appdata\local\programs\python\python38-32\lib\asyncio\events.py", line 501, in add_reader
raise NotImplementedError
NotImplementedError
Can we have a review on https://github.com/python-discord/seasonalbot/pull/386
cuz its been more than a month since https://github.com/python-discord/seasonalbot/pull/349 started.
I mean there are PR that are pending for more than 6 months but this one is a small and easy to review ;-;
brazen charm
@weary rampart aiohttp uses a lib that's not compatible with the current default asyncio event loop (on win)
you can set it to the event loop that works with asyncio.set_event_loop_policy(asyncio.WindowsSelectorEventLoopPolicy())
I have that in sitecustomize.py under my install's site-packages for bot
(That gets ran through site)
weary rampart
Ah - thanks
a nod to that would be very helpful in the wiki https://pythondiscord.com/pages/contributing/seasonalbot/
brazen charm
I think there has been some small discussion about doing it in the code itself (for bot) but no issue has been made afaik
cold moon
Can anyone review my site PR (is this implementation okay)? I don't write test for it before I know is this good.
mellow hare
@clever wraith Was 349 not closed because you yourself said it was kind of a mess? But in the 386 you're saying that it's essentially the same PR?
I guess I'm confused what makes these two different
clever wraith
the 349 one became outdated fork so i made a new one , cuz there were difference in flake8 that was causing linting to fail
386 is addition to 349
tawdry vapor
@clever wraith it's kind of a mess and I have a local branch that tried to alleviate some of the redundant configuration. The log is passed cause the temporary file is dynamically created, the cgroup stuff is passed cause there's no guarantee currently that the cgroups it creates will be the same ones defined in the config. The python path and options are passed cause IIRC nsjail was not respecting the ones defined in the config due to the aforementioned additional options being passed along with the config file
clever wraith
off the top of your head, do you know if it respects the pid count set in config files?
tawdry vapor
Yes
That one doesn't need to be set again
Probably just forgot to remove it when moving things over to the config
tough imp
@clever wraith I should have some time to look at your PR today or tomorrow if no one else does by then
tough imp
keep in mind that commit messages remain important once the PR gets merged as well - you may want to look at why, when or by whom a certain change was made
clever wraith
sure it does
tough imp
so just because the commits in your old PR had descriptive messages doesn't mean that the new ones don't have to
the old one won't be getting merged, the one ones will
tough imp
I don't think so, you'd have to force push if you change the commits, and you probably don't have perms to do that
also where is that heart in your name ?
it's so that every time you mention me you have to send me a heart
clever wraith
I liked that name . it give a different look sad its gone :C
molten bough
I see a heart
patent pivot
looks normal to me
clever wraith
I thought hearts are unicode
mellow hare
Not all fonts have all of the unicode covered, however
cold moon
For me too