I added a new test and I get a random test error
#dev-contrib
hardy gorge
Which error did you get?
May be worth investigating why it failed randomly
@brazen charm Not terribly difficult and I hope that there will be a couple more examples available in the near future.
molten bough
Try not to stretch yourself too thin @green oriole :>
green oriole
What does that mean?
molten bough
try not to do more things at once than you can reasonably handle
molten bough
Don't you have a bunch of PRs awaiting review?
green oriole
BTW I just want to point out that I did 5 consecutive pushes that all passed linting :)
Yeah
But they are all finished
molten bough
are you sure those are 100% ready to go? can you improve them?
I know staff are too busy to do tons of reviews right now so you could take the opportunity
:>
green oriole
Yeah I did some improvements when I have ideas
It's holidays until the end of October so I try to do has much work as I can
They are all little PRs
crude gyro
sorry, it's hard to keep up with the sheer volume of PRs we've seen this month
molten bough
Haha, I don't doubt it, it's fine
hardy gorge
@green oriole Please be careful with force-pushes to branches you've already put in a PR for. It will make reviewing them a lot more difficult, since it disconnects the review comments from the code. In general, it's probably best not to force-push to branches that already have a PR, unless there is a pressing need.
molten bough
My policy is just to never force-push unless absolutely necessary
hardy gorge
That's probably the best policy for code that's publically available to avoid conflicts in the local history of other people as well, although we rarely branch from or work on branches pushed by others.
I don't mind it that much if there's no PR associated to a branch, but, in general, it's best to make sure your local history is clean before pushing it to remote
crude gyro
yeah exactly. just rebase your commits or whatever before you push them the first time and no force push is needed.
but that said, it really isn't a big deal to force push to a branch that there's no PR for and nobody else is working on.
and I don't think that you really need to add a โ emoji every single time that happens, @molten bough
I get that it's not your particular workflow, but it is acceptable.
molten bough
Oh, I know, I was just messing around
crude gyro
yep yep
hardy gorge
@molten bough Is there a way to test your allauth PR locally? I'm not sure how to get past DoesNotExist at /accounts/discord/login/
green oriole
Okay sorry I just wanted to make the history very clean
molten bough
@hardy gorge I'm not sure why you're getting that
Have you set up a provider?
You do it in the admin panel, social applications
hardy gorge
Oh, right, that's probably missing
molten bough
Aha
mellow hare
Good band
molten bough
Social Applications? Haha
hardy gorge
Aha, probably
mellow hare
^
molten bough
Aha!
hardy gorge
I'm going to need to figure out how to set that up properly. I'll get back to it later.
mellow hare
They did Take On Me
molten bough
They did, yep
:>
It's not super hard, ves
basically social applications are oauth apps
so you say, discord, client ID, client secret
oh, and you need to set the oauth url in the oauth app
it's http://pythondiscord.local:8000/accounts/discord/login/callback/
you can probably guess what the github one is, haha
green oriole
https://github.com/python-discord/bot/pull/617 @woeful thorn you told me to use a Task, @hardy gorge you told me to use a Scheduler, what should I do?
I think a Task would better fit here right?
hardy gorge
I don't see the value in adding another "do this after [x] time has passed" method.
I'd say we use the Scheduler we have, which does exactly that. It's used for infractions (remove after [x] time) and reminders (show after [x] time), which fits the "delete after [x] time" model as well
Unless I'm overlooking something, I prefer us to use the same method for the same type of tasks instead of reinventing the wheel every time we do it
If we just use the message timestamp/datetime + 7 days, I'd say we're good
mellow hare
Agreed
green oriole
So do I still need to store them in the db?
woeful thorn
Yes
woeful thorn
You don't really need reverse engineering, both the notifications and the infractions cog have on_ready methods that reschedule the tasks when the bot restarts
green oriole
I mean, I need to understand how these cogs works
Because functions are a bit all over the place for some of them
mellow hare
Just take your time with it, you'll do fine
green oriole
None of them seem to have a on_ready method
woeful thorn
https://github.com/python-discord/bot/blob/master/bot/cogs/reminders.py#L35
https://github.com/python-discord/bot/blob/master/bot/cogs/moderation/infractions.py#L49
Sorry, wait_until_ready
woeful thorn
Seems like it would make things easier, wouldn't it?
hardy gorge
@green oriole The logic for the scheduler shouldn't be too difficult. The base class lives here: https://github.com/python-discord/bot/blob/master/bot/utils/scheduling.py. You only need a couple of methods to use it, like saving to the database and scheduling the task, making sure tasks are rescheduled when the bot reconnects (sync with db, think about what should happen if the datetime is now in the past due to some downtime), and an overwrite of the abstract method that actually gets called when we get to the scheduled time.
green oriole
Yep thanks!
green oriole
https://github.com/python-discord/site/pull/300 @hardy gorge i'm a bit lost on that one ๐ you said you want me to write tests for my viewset, what should I do? I found this file https://github.com/python-discord/site/blob/master/pydis_site/apps/api/tests/test_models.py I just need to add a new entry in StringDunderMethodTests.object?
hardy gorge
@green oriole No, not exactly. What I meant is that when you look at the API you've designed for your "offensive messages" model, it has certain properties, like how it should respond to GET methods (and which params it should take), how it should respond to POST methods, that it should (or should not) accept DELETE methods. There are als requirements for the data, like non-negative message and channel IDs and things like that.
What you write it tests that test if that behavior is actually being displayed by the API endpoint
That means that if there ever is a change that breaks that API, it will show up in failing tests
From a test-driven development perspective, you could also first implement the tests: "If I send this kind of request, this should be the result" and then implement in such a way that the test passes.
There are a couple of examples of such tests, but not all endpoints currently have them.
I think we should strive to have them, though, and add them for new models and views at the very least
A couple of example test files:
hardy gorge
Okay, the __str__ method of your model determines how your entries are going to show up in, say, the admin panel of Django
So, making sure it has a desent string representation helps people looking at those those entries in the backend
I think test_model, from memory, just looks at if that string representation returns a str
Still, you probably want to think about how you want your entries to be displayed to humans
green oriole
I think you left a comment about the admin panel, what is it? A way to administrate the db?
hardy gorge
It's the thing you see when you log-in to admin.pythondiscord.local:8000 (for a dev site)
green oriole
It requires a login
hardy gorge
Yes, admin:admin is the standard for the dev set-up, but, since you're not using the standard dev set-up, it may be that you need to manually create a super user using manage.py
Not sure
molten bough
Nah, it should've made it
Scrags integrated it with a custom manage.py
--debug does that stuff
hardy gorge
Anyway, this file registers models to be displayed in the admin panel: https://github.com/python-discord/site/blob/master/pydis_site/apps/api/admin.py
ah, nice
green oriole
Yep admin:admin works
hardy gorge
So, the models don't magically appear there, but only if they're registered to it. It's a single line (in the file I linked above)
I need to go to bed, but I think you'll figure it out, since you've managed to figure out most stuff
green oriole
Yep I'm gonna try thank you for your time ves
patent pivot
python discord contribution from feb 2018 to now, site, bot, seasonalbot and snekbox
glass pecan
Nice
tawdry vapor
I liked watching people be dragged into the void and slowly disintegrate
hollow lichen
Nice! I saw myself, yeah!
green oriole
I love how things literally explodes in October 2019 ๐
On every page of the site (at least on mobile) the page name is written a second time
green oriole
When launching bot:
future: <Task finished coro=<Reddit.init_reddit_ready() done, defined at E:\PyDis\bot\bot\cogs\reddit.py:39> exception=Forbidden('403 FORBIDDEN (error code: 50013): Missing Permissions')>
Traceback (most recent call last):
File "E:\PyDis\bot\bot\cogs\reddit.py", line 43, in init_reddit_ready
self.webhook = await self.bot.fetch_webhook(Webhooks.reddit)
File "C:\Users\Akarys\.virtualenvs\bot-rhzqH2X-\lib\site-packages\discord\client.py", line 1265, in fetch_webhook
data = await self.http.get_webhook(webhook_id)
File "C:\Users\Akarys\.virtualenvs\bot-rhzqH2X-\lib\site-packages\discord\http.py", line 218, in request
raise Forbidden(r, data)
discord.errors.Forbidden: 403 FORBIDDEN (error code: 50013): Missing Permissions```Can we disable the connection to reddit if DEBUG_MODE is on?we're actually doing the opposite lol
we're enabling everything on debug mode soon
just create a webhook and add it's ID to the config
green oriole
Well, at least not try to connect if there is no credentials
hardy gorge
This is not a connection to reddit
glass pecan
hardy gorge
This is a new webhook for the reddit channel
green oriole
Oh yeah
hardy gorge
You need to create one/set the id in the config
tawdry vapor
Can't wait for a new config system :D
glass pecan
haaa, yeah
lol
i should try do that by the end of next week
need someone to whip me occasionally to make sure it's done 
lol
glass pecan
writing plans!? what do you take me for, a sensible person?
tawdry vapor
I don't remember it all myself
glass pecan
but yes, we should make an issue ticket
tawdry vapor
Well at least the message history exists
glass pecan
true
afaik it's mostly autosetup, config file generation, server deployment if empty
also adding emoji
crude gyro
... new config system?
glass pecan
well the config needed some work
crude gyro
guess I missed that discussion
glass pecan
regarding the defaults inheritance
you'd have seen that talk at least
there's an issue ticket on it
crude gyro
not sure
glass pecan
you and I also talked about it ages ago when i was fixing the yml config setup
crude gyro
well catch me up
glass pecan
was my first commit i think
well, currently we have two config files
ones the defaults
if you alter a field that acts as a variable for other ones
since it's a yml variable, you need to copy every child field also
crude gyro
...you do?
glass pecan
yes
crude gyro
oh. that sucks.
green oriole
Oh yeah and that's horrible to do ๐
crude gyro
I wasn't actually aware of that
glass pecan
so we figured there's two ways of fixing this
stop using yml variables
and having the script apply the inheriting values
so we'd only need to plop the top level field in the customised yml
or we could have defaults in our constants.py
and just have a single config file
crude gyro
stop using yml variables, by which you mean, the groups?
so like flatten the whole config?
glass pecan
no
urls:
# PyDis site vars
site: &DOMAIN "pythondiscord.com"
site_api: &API !JOIN ["api.", *DOMAIN]
site_paste: &PASTE !JOIN ["paste.", *DOMAIN]
site_staff: &STAFF !JOIN ["staff.", *DOMAIN]
site_schema: &SCHEMA "https://"
here's a great example
&DOMAIN is a yml variable
crude gyro
right. not really but yes it is.
glass pecan
if we change site, we need to copy all child fields that relies on site
including the api, paste, staff parts
while if we did this part in the script, we'd just have the urls: site: lines
green oriole
(that's could be cool also to have a dev environment ready config, with things already set to http, pythondiscord.local, port 8000...)
crude gyro
yeah I got you @glass pecan
I was just confused because YAML calls them node anchors, not variables.
conceivably a variable could be a sequence, a scalar, whatever. but I'm with you now.
but are you then suggesting to completely ban node anchors from the yml file?
because, I mean, they significantly help readability in some cases.
green oriole
tbh that was the first time I worked with yml file, and they aren't strait forward at all imo
crude gyro
I can see that.
now that I'm looking through it, I guess we could move all the various places that we use these features into the constants file.
glass pecan
I don't intend to reduce readability, as I'm sure we can implement this variable thing script-side
if needed
crude gyro
you keep saying script
crude gyro
it's confusing because I don't know exactly where you plan to do it
and because you mentioned a setup script
glass pecan
in the same place we load the yml
crude gyro
so constants.py
glass pecan
i haven't mentioned the setup stuff at all since i have started the "catch me up" thing
crude gyro
that's true. you mentioned it like.. one line before that
glass pecan
yes, so i gotta catch you up before touching that
lol
so there's no set decisions yet on the approach to fix the yml inheriting value thing
but it was the first thing that brought up the consideration that we need to fix up the configuration aspect a bit in bot
crude gyro
so, abolish node anchors and do all constant processing in constants.py. I think that would probably be an improvement after looking through it a bit.
glass pecan
that's the general idea, yeah
but we don't know yet if we want to
a) keep defaults in config-defaults.yml
b) put defaults in constants.py
crude gyro
hm. right.
glass pecan
atm, i find there's a pretty big reason to keep defaults in a yml
so we can copy and paste parts into a child config
glass pecan
however
crude gyro
that's the major thing
glass pecan
since we're there, i figured why not enhance how we configure our dev environments anyway
if we can run a setup command to autodetect existing channels, create new channels, and create required things such as roles, emoji and webhooks, we can have it generate the child config anyway
if 99% of people ended up using that, is there a major reason to keep the defaults in a yml anymore
the setup commands would live in a development only extension that's loaded only when debug mode is on
and it would need to be explicitly called and confirmed
and there would be individual commands for individual settings when needed also
for example, if we setup a server and later added a new channel, you could just create that channel and declare it as that setting value
crude gyro
okay. sounds extensive.
glass pecan
extensive but highly useful
it wouldn't be terribly hard to do, it's just a bit of time
crude gyro
and this autosetup thing would work fine without moving any defaults.
glass pecan
yes it wasn't about making that part easier
it was just about removing another unnecessary file
crude gyro
right. yeah but other than just the copypaste argument, there's just the simplicity of the thing.
essentially if we have defaults and overrides defined in two different ways, it's more complex than if they are defined exactly the same way
glass pecan
brb, keep going, gotta get a drink
crude gyro
I just think it's easier to wrap your head around the idea that config.yml is a subset of config-defaults.yml when you can open them both up and see the relationship between them spelled out.
glass pecan
yeah that's fair
crude gyro
I'm not violently opposed to it but I don't think that "removing an unneccessary file" is adequate justification to make the system even 0.1% more complex.
but other than that I think everything you said sounds very reasonable
so, for what it's worth, you have my support for all of those ideas.
and also, uh, to lighten the load, I guess I'll stop using node anchors for any upcoming PRs of mine.
glass pecan
nice, always worth it
and no stress to use them if it makes sense
since we'll need to replace that with something equivalent
crude gyro
I think either we use them or we never use them.
and if we never use them, we just create all the groups and stuff inside constants.py
the downside is just that then we're splitting up constant creation between two files
glass pecan
i don't follow
crude gyro
consider this example
# Censor doesn't apply to these
channel_whitelist:
- *ADMINS
- *MODLOG
- *MESSAGE_LOG
- *DEVLOG
- *BBLOGS
- *STAFF_LOUNGE
- *DEVTEST
- *TALENT_POOL
- *USER_EVENT_A
yep
crude gyro
if we wanna change which channels we whitelist now, we have to do that in constants.py with this new system
so like, some changes will need to be done in the yml, others in constants.py
glass pecan
nah
crude gyro
whereas now we can (should be) keeping everything in the config file.
crude gyro
how would that work
glass pecan
i still haven't wrote or even looked into the available tools in yml properly to be able to give you an immediate set solution
but the idea is that you'd just not use anchors
crude gyro
oh. you're saying we roll our own solution?
glass pecan
yes
glass pecan
it'll still be inheriting
it just will inherit across ymls
like how we roll our own !ENV thing
I'm sure we could do that
crude gyro
yeah this is possible, the !JOIN tag is custom built.
yeah
yes it's possible to roll any constructors we want
glass pecan
i just can't give you anything specific because i don't know what it'll look like at the end
but if it means a tag, that's simple enough too
glass pecan
very doable
but
i don't want it just doable, it needs to be sensible and clean
lol
crude gyro
not sure it'll be any less confusing for new contributors though
ref what @green oriole just said
glass pecan
in what sense
green oriole
If we use a more clear notation that will be fine
glass pecan
more clear notation?
green oriole
Like, I don't know, what about a dot notation like
channel_whitelist:
- channels.admins
- channels.modlog
- ...```Something that people that do python would easily understand
glass pecan
i feel like that's kinda fighting against what yml is
green oriole
Maybe yml isn't a good choice then?
crude gyro
lemon: !VAR "ding dong!"
melon: !REF lemon
Here's one way that I could imagine it working, like, within the technical confines of the framework.
but this is undeniably more complex.
glass pecan
yeah, i thought !VAR made sense too as a tag name
green oriole
That as confusing as anchor node IMO
glass pecan
sounds like you just don't like yml then yeah lol
crude gyro
it's probably more confusing than node anchors - which, for the record, I don't agree are confusing at all, I think they're a very sensible way to do it
green oriole
No I like it, once you understand how it works it's fine
glass pecan
i don't really think a var and ref tag would be that confusing 
the issue is the notation of node anchors are entirely unfamiliar to those who don't know yml
green oriole
Yeah that's the problem I think
tawdry vapor
Has it been decided to stay with yaml even? I know toml was discussed but don't remember the conclusion of that, if there even was one
crude gyro
maybe. but I think a quick scan through the yml file makes it immediately obvious how they work
glass pecan
yeah there was a discussion about toml mark, but i don't think it got much headway
crude gyro
isn't toml just yaml minus the features we're using?
green oriole
toml is a standardized ini with more features
glass pecan
and has no extra features at all that yml has
glass pecan
because it's not the same syntax at all
glass pecan
yaml doesn't seem very standardised to begin with apparently
their spec is huge and a lot of implementations apparently differ
that doesn't impact this though
crude gyro
I like them all.
glass pecan
in my head though i just consider yaml a fancy json without the brackets
tawdry vapor
I'm ok with yaml I think
glass pecan
do we have any strong opinions from others?
tawdry vapor
But toml syntax is preferred
glass pecan
might as well make an issue ticket to make sure people get an opinion in
tawdry vapor
Not sure how it would impact the config features though compared to yaml
glass pecan
ive only been using toml lately thanks to pyproject
crude gyro
my one major question is again back to this example:
# Censor doesn't apply to these
channel_whitelist:
- *ADMINS
- *MODLOG
- *MESSAGE_LOG
- *DEVLOG
- *BBLOGS
- *STAFF_LOUNGE
- *DEVTEST
- *TALENT_POOL
- *USER_EVENT_A
I don't see how this would be possible at all in toml.
maybe if we subclassed the toml parser with custom parsing.
somehow
and having this be a list of IDs kinda sucks.
as does handling it in constants.py
crude gyro
one more thought, though, @glass pecan
glass pecan
ye?
crude gyro
if we did have that autoconfig command to set up the config file automatically
would this inheritance thing matter at all?
glass pecan
yes
it'll be less so, but it's not like people won't quickly edit or add a field in config when the bot is off for some reason
it makes things easier, but it's not a solve all unfortunately
crude gyro
I mean so long as the file was set up with with every single variable (instead of just a subset) your edits would work
glass pecan
it also makes it easier to generate the config file
since i don't have to go finding all the anchors used
and copy the child fields
the idea isn't to just copy the entire config
it's to show only things that aren't default
crude gyro
no, but that could be the idea.
because what does it matter when it's done automatically
glass pecan
i dunno about you, but i find it a pretty strong reason for being able to open my child config file and see exactly was nonstandard/nondefault in my setup
atm, that's nearly impossible
crude gyro
okay. I guess I buy that.
I don't think I agree it's a very strong reason but it's definitely a reason.
anyway okay, thanks for catching me up
glass pecan
no probs, i had to remember it all anyways
crude gyro
I guess we'll see what we can come up with
tawdry vapor
Probably but I wouldn't remember either
glass pecan
lol
tawdry vapor
If you have the time I suggest you try to find the previous discussion and go over it
glass pecan
i don't atm, but i'll try do so tonight
tawdry vapor
If you don't by the time you make the issue I can do it and edit or comment
tawdry vapor
Just let me know if you get to it first
green oriole
Just, I browsed a bit the Internet about toml variable and the creators clearly said that no variable logic was designed, because you're free to implement your own system after the file has been parsed, for example create a dot notation.
crude gyro
yeah that doesn't really help us
molten bough
Wasn't that the plan with the yaml files?
I mean I thought the major problem with it was that it has that stuff built in
crude gyro
feel like you're missing the context. we just had a really long conversation about this
molten bough
I might well be, yeah
crude gyro
molten bough
Okay yeah, I'm caught up
I'm just thinking that if we're rolling our own.. What are they called in yaml? Constructors?
I know toml doesn't provide support for them but if we're only using scalar values then I can't imagine it'd be hard to do it ourselves
On the other hand, I'm not necessarily a big supporter of toml and it wouldn't be good if we ended up modifying it to the point of us having PyDis Config Language
Another suggestion I have is.. Well, why not Python?
The bot isn't targeted at people that can't write basic Python after all
At that point inheritance can just be subclassing a dataclass or something
crude gyro
hmm. it would probably help reduce complexity if we removed the markup from the equation completely. The config files could essentially just look like constants.py with defaults, then.
to be fair, our YAMLGetter class and the custom constructors are fairly arcane.
molten bough
It'd be fairly simple I think
Constants has a DefaultConfig class
And then have a config.py which can be wherever that inherits it
crude gyro
yeah.
well, a little bit more complex than that but that's definitely the right idea.
constants would also have to figure out which config to make available for imports
but none of it would be hard to do
@glass pecan @tawdry vapor thoughts on this approach?
glass pecan
i don't like the usage of a script for configuration, i also don't like the idea of having to construct a class in a child config
that's my thoughts
molten bough
Why so?
glass pecan
as a general rule, configurations should be read-only
making it a python script means you're executing code
I don't like encouraging that
molten bough
Configurations aren't read only though
glass pecan
the config file should be
molten bough
They're just a file on disk and you could feasibly monkey patch or edit the data in memory
glass pecan
"edit the data in memory" is a very crappy excuse because that applies to practically everything
molten bough
Sure, but a class is also, yknow, a file
You're not editing it by loading it, right?
crude gyro
I don't understand your argument at all, I figured you'd have a practical reason not argue on some principle
glass pecan
i don't think i need to reword my statement to make it more understandable
molten bough
I don't really understand it either
crude gyro
I mean aren't we doing exactly this on seasonalbot?
glass pecan
seasonalbots variable configs should be using environemental variables
molten bough
I just can't see a config file being harder to edit at runtime than a Python file
Especially considering a Python file is harder to serialise to and we review all PRs anyway
glass pecan
A more typical configuration file is more secure and protects against any random code from executing at the point of import. Yes, in our projects it might be fine, but as a rule I don't like encouraging it, and since people look to our projects as examples, I find that an important aspect to consider.
molten bough
I disagree with that notion on principle
crude gyro
I don't. I think that's fair.
molten bough
There are quite a lot of big and popular projects that do this
Django and moinmoin for example
The latter of which powers the entire PSF wiki scene
glass pecan
I don't agree that just because a big project does it, it should be fine for everyone.
It's ignoring the reasons
molten bough
Sure, but I can't imagine they didn't think of security, they're webapps after all
The only thing I can think of that might be a problem is a malicious actor, like a virus, modifying the config script
But we're using virtualenvs anyway so they can just hit those
glass pecan
No a virus does not need to be involved in cases where someone has a project, someone tells them to test a config and then that config executes malicious code
molten bough
That really just comes down to, don't run untrusted code, doesn't it?
glass pecan
yes, but why allow the giant opportunity in the first place
crude gyro
let's go ahead and disengage this now, please. you both have valid points.
we'll continue the discussion regarding a Django-esque config file vs a yaml file or a toml file internally, what precedent we're setting is a valid consideration, as is overall complexity. it's tricky, and it's clearly evoking strong opinions so lets not have this devolve into a fight.
might be staff meeting fodder.
we'll see.
thanks for the input though, I think we've got more research to do and more opinions to collect before we make the final call here.
molten bough
Okay.
green oriole
Guys, I created a view at problem solvedhttp://api.pythondiscord.local:8000/bot/offensive-message but then I do url = reverse('bot:offensive-message', host='api') in my test suite, I get a NoReverseMatch exception. Any idea why?
molten bough
Is it in the bot app?
green oriole
Yep
molten bough
Hm, so you've written a bot app and assigned it to the API host?
I though there was just an api app
molten bough
Sorry, I'm not sure how the specifiers work
grizzled inlet
Would this be a right place to ask about snekbox?
$ sudo docker pull pythondiscord/snekbox:latest
$ sudo docker run -ti pythondiscord/snekbox
....
File "/usr/local/lib/python3.7/pathlib.py", line 1258, in mkdir
self._accessor.mkdir(self, mode)
OSError: [Errno 30] Read-only file system: '/sys/fs/cgroup/pids/NSJAIL'
....
Just about my previous question, I dig a bit in the source code and it seem like it is assigned to the namespace problem solvedbot
@grizzled inlet did you follow the instructions here https://github.com/python-discord/snekbox/blob/master/README.md#development-environment ?
grizzled inlet
Tried both, setting it up from repository with the pipenv commands to build docker image, ended in the same place as pulling the image from docker
nvm, running it without docker works
green oriole
If you do pipenv run builddev and docker-compose up you get the same error?
grizzled inlet
@green oriole I will have a look later, got other stuff to do, but thanks!
This is what happens with basic (non-dev) setup after doing all three pipenv run ... under Running snekbox, and running docker-compose up
ERROR: for snekbox_pdsnk_1 Cannot start service pdsnk: OCI runtime create failed: container_linux.go:345: starting container process caused "process_linux.go:430: container init caused \"rootfs_linux.go:70: creating device nodes caused \\\"open /var/lib/docker/overlay2/012e1313c42c22e9be3a3b85743bae558e3030b51040be1a92ccbb5d1c50a06c/merged/dev/tty: no such device or address\\\"\"": unknown
ERROR: for pdsnk Cannot start service pdsnk: OCI runtime create failed: container_linux.go:345: starting container process caused "process_linux.go:430: container init caused \"rootfs_linux.go:70: creating device nodes caused \\\"open /var/lib/docker/overlay2/012e1313c42c22e9be3a3b85743bae558e3030b51040be1a92ccbb5d1c50a06c/merged/dev/tty: no such device or address\\\"\"": unknown
ERROR: Encountered errors while bringing up the project.```trying builddev
The same error happens, even after doing docker system prune
grizzled inlet
Distributor ID: Ubuntu
Description: Ubuntu 18.04.3 LTS
Release: 18.04
Codename: bionic
I bet the VPS did, reboots every 0:00
molten bough
Weird, but okay
grizzled inlet
12hours uptime
molten bough
You have the docker service running I take it
grizzled inlet
yes
molten bough
Do you have it configured to use a Unix socket?
molten bough
Okay, what's the rest of the traceback in your message here? https://discordapp.com/channels/267624335836053506/635950537262759947/637617422702215188
grizzled inlet
$ docker run -ti pythondiscord/snekbox snekbox
grizzled inlet
nope
molten bough
Or docker's own PPA?
grizzled inlet
ppa
molten bough
Hmm, that's correct
molten bough
Can you check what user the docker process is running under as well?
grizzled inlet
the socket ownership is root:docker
grizzled inlet
I have resolved it a bit differently (luckilly the VPS is kind of throw off). I didn't hasle with docker, built nsjail myself put it in sbin and run snekbox.
Happily evaling my python code from curl
grizzled inlet
Damn, I didn't pin the packages, seems the ubuntu version was installed
pruning, trying again
aand it builds
damn, thanks @molten bough
molten bough
Ding ding, no problem
hollow lichen
This might be slightly off-topic but my issue is related to the bot-repo organisation.
I'm trying to create a discord bot mirroring the bot repo (content ofc will not be the same), but without site dependance. So instead of api.py I'm writing db.py where I define all my functions related to my database. and then I import these functions in my cogs to use them. Question is: How to avoid opening and closing the database in each function of db.py? Or is it ok to do so?
exotic ember
@hollow lichen I'm not database savvy, but you would normally have a connection pool created upon bot startup, then acquire connections when you need them
exotic ember
the emoji stuff is proving to be a bit of a pickle, since it's a custom emoji so I can't use it directly for reactions (please smack me with how if I can) and have to instead use get_emoji with the emoji id
which is fine and dandy for the wait_for_deletion helper, but is a bit of a janky pain in the help cog
tawdry vapor
Why is help a problem
exotic ember
the current reactions to be added are in a module level dictionary, with the keys being strings \u8294adswdwa
I need the bot to use get_emoji, so I thought to add the custom emoji to the dictionary in __init__, which is fine and dandy for adding the emoji to the message
however, on_reaction_add works by str()ing the reaction then checking if that string is in the REACTIONS dictionary, which won't work if I add my custom emoji as an Emoji instance to the dictionary
exotic ember
that I can do, hence the jankyness
tawdry vapor
Itd be awkward for the emoji to be the key rather than the value though
Well, so be it janky
exotic ember
the cog is awkward, the reason the emoji is the key is because the cog uses get_attr with the emoji's value to determine which method to run
tawdry vapor
I think there's a PR which reworks all that anyway
exotic ember
ah there is, I shall wait for that then
tawdry vapor
I don't think you have to but if you wish you can
It would be good to ensure the pr design accounts for custom emojis
jade tiger
nvm
ill do some reading before i start spouting nonsense
@exotic ember a bit more context? unless I'm thinking of something wildly different can't you just do add_reaction(constants.Emoji.blah)?
exotic ember
the emoji is a custom one, I've tried doing that but it doesn't recognize the emoji
exotic ember
maybe I had the id wrong, I'll try again later
green oriole
#dev-test is still in the config file, so #deleted-channel appears in the wrong channel message of @dusky shore
exotic ember
gotch
green oriole
@exotic ember your last commit (56696b3) has been done in another PR and has been merged already, you are going to get a merge conflict :)
exotic ember
yeah I realized, I'm not done with it though luckily
green oriole
I just started working on a issue for the antispam cog, and the code seem old (more than 1 year), like it still use the 76 chars limit, do a lot of API calls and can be pretty hard to read. I would like to attempt to rewrite it in the most part, can I?
green oriole
The functions inside bot/rules/ they haven't been changed, and try to do some optimisations, like some caching, to avoid doing hundreds of API calls, and try to run the rules at the same time using asyncio.
And restructure the code a bit to make it more clear
woeful thorn
What rules make hundreds of API calls?
hardy gorge
asyncio will not make the rules run concurrently, that's not how asyncio works
Asyncio still does one thing at a time, it just switches between tasks on the programmer's terms
Instead of on the terms of a thread manager (with Python threads)
green oriole
If I'm not wrong every time the rules runs the cog fetch all the recent messages @woeful thorn
hardy gorge
That is true, yeah
You could fetch the longest antispam rule period, cache those messages, and select the ones relevant for the specific rule
green oriole
We can also put the rules and the cog in the same folder, that wasn't what you are trying to do with all cogs?
woeful thorn
Doesnโt the bot already cache messages?
green oriole
I'm gonna re-check but I'm pretty sure it clearly ask discord for a new history everytime
gusty sonnet
Yep I'm doing a search on all on_message event as well as searching for .history() the bot is still using discord api calls
There doesnt seem to be any cache
We can easily implement a deque of max 5k per channel considering we dont have that many
woeful thorn
The client has a built in message cache
gusty sonnet
tbh the .history call isnt calling for hundred of messages
It does, which is wiped clean every restarts
woeful thorn
Ok, but why cache twice
crude gyro
any cache we could implement would be wiped clean every restart unless you're proposing storing message history in the database.
green oriole
I checked the source and it doesn't seem to do any caching
gusty sonnet
This is a weird structure in the source
But yeah from what I see
only non async stuff like get_user and stuff are calling into the cache
message cache isnt actually useful inthe source, all .history is calling from the discord api
every single call
Is there a limitation on our current database?
If not we can actually make a table for storing most recent messages
Let python deals with it with its deque and its max default, background process updating database every x time
crude gyro
there are privacy implications to storing a ton of messages in the database that we may need to consider.
woeful thorn
I donโt see any compelling reason to make spam filtering dependent on the database
gusty sonnet
Like all message from an user instantly, across 5 channels
green oriole
Why we don't just cache in the RAM, the bot isn't restarted that often
gusty sonnet
Ah no spam shouldnt be dependent on the db
I'm thinking of a way we can help spam filter to be more efficient and avoid api calling
It can rate limit the bot
If we cache in the ram it suffers the same problem, it's wiped clean every restart
gusty sonnet
It wont if we do extra checks if cache has less than x amount of message
In that case we fill this RAM cache
green oriole
That would still be better than the current solution
gusty sonnet
True
crude gyro
so, caching in memory seems unproblematic to me
but caching in persistant storage is a path I'm not sure I want us to walk down unless we really have to.
gusty sonnet
Eitherway we will maintain a cache in the memory anyway
So we can and should go down that route first, it is a great proposal for the current way it is working
crude gyro
if it's slow once every restart, that's a small price to pay.
gusty sonnet
Storage can come later, it isnt as important
gusty sonnet
I'm more worried at the bot being rate limit and being forced offline / unable to send message
crude gyro
sure, but I haven't seen any compelling evidence that that's likely to happen
glass pecan
we've never had that issue and we have a lot of more simple things we could do to lessen the load later other than this
crude gyro
every time we've looked into rate limit compliance we've found that we're so far inside the rate limits that it's not even funny
glass pecan
perm message storage means a change in our privacy terms and we'd be required to store our db on an encrypted drive
it's not a small thing
we'd also need to create things in a way where specific member data is purged on request
i'm not a fan of the idea
molten bough
There's a ton of overhead to that, yeah
crude gyro
but caching in memory comes with pretty much none of these concerns.
gusty sonnet
I mean, right now it is calling from the discord api, we can always build on it and rely on the current as the fallback
I completely forgot about privacy until lemon mentioned it lol
crude gyro
basically I think if you built a robust caching system that helps us prevent API calls for message history (which is by far the most expensive API operations we do), then we'll probably be happy to merge it. Just so long as there's no persistant storage involved.
we all agree with that?
gusty sonnet
The problem is .history doesnt call into the cache inside the lib
molten bough
From my experience working with the cache directly, d.py's internals can be a total mess of timing
glass pecan
i'm aware of what history does
i think it should still be considered gdude, as doubling the cache for little reason without actually attempting to use the existing one to see the specific challenges just doesn't come across smart
molten bough
I agree, but it might be tricky
glass pecan
we all know how much you hate dpys internals, but i don't really agree it's as bad as you make it out
molten bough
The cache is often not in the state you'd expect it to be
gusty sonnet
It will be tricky to access and use the cache exactly like how the lib uses
And we'll now be reliant on how they use it
crude gyro
wouldn't we need to monkey patch the history calls in order to have them make use of the cache?
glass pecan
why would you need to patch that when you can make a specific one
shouldn't throw out a valid tool
molten bough
Wouldn't it just fall back to API if it isn't in the cache anyway?
green oriole
That was the idea
molten bough
I could be 100% wrong but that doesn't sound breaking
but it might be worth finding out why it doesn't use the cache in the first place
glass pecan
because it's designed to not use cache
glass pecan
it's specifically to get fresh message data
green oriole
Because you aren't supposed to call history every time someone post a message
glass pecan
there's no reason to entirely replace that call, since if we ever needed it in future, we'd just have to undo it
molten bough
hm, I guess so, yeah
glass pecan
it's also still going to be used as-is anyway as the fallback
at any rate, the idea of checking cache first is fine, I think it's a good idea. it will have quite a few considerations though and wouldn't really be something considered beginner or intermediate level
gusty sonnet
You mean the internal cache, right?
glass pecan
either or
my previous statement was to at least check the possibility of using the existing internal cache
rather than just assume it's not to be touched and that we should for some reason duplicate the cache again in another collection
molten bough
I don't think anyone said that, but alright
gusty sonnet
Hmm, py self.max_messages = max(options.get('max_messages', 5000), 100)I just realized they use the same thing
self._messages = deque(maxlen=self.max_messages)```With the sheer amount of messages in the off topic channels, this cache can be completely useless
molten bough
I mean, you can certainly raise that
gusty sonnet
Even though we still do check the off topic
molten bough
I remember doing that in the old bot
gusty sonnet
Well yes, we totally can
molten bough
Do you feel like it'd become a bit of an arms race?
woeful thorn
The longest time window is 10 seconds
Do we get more than 1000 messages in 10 seconds?
molten bough
We've had 15k in the previous 48 hours or so, so.. nope
hardy gorge
If want a cache that's used for more than a single antispam on_message (like a shared cache between calls), the implementation gets complicated quickly (as in, we need to make a lot more design calls). Things like how we handle messages that were deleted in the mean time, since they will still be in cache, but not actually in channel. Ditto for edits. Obviously not very difficult problems to solve, but it is a more involved design problem that a simpler "just cache this specific history call for the duration of this on_message" handler.
It may be worthwhile to thing about our central "on_message" processing design, which would fit into the Bot subclass idea @tawdry vapor has proposed.
We currently see side effects of using different on_message handlers because the order is not guaranteed, so the "codeblock" formatter may duplicate a message that's going to be deleted by the token remover or a filter
By having a central on_message that fires off these things, like RoboDanny, we can fix the order in which these rules are applied and only process commands once we know it's not a message we don't want to process
green oriole
Wouldn't be worth it to implement a cache only for the antispam cog in the first time (since it do way too many API calls that it needs to) and maybe use this code to create a global cache later, since history is only used in 3 other cogs?
woeful thorn
"way too many API calls" is not echoed by Discord. I don't even think we hit rate limits on the filter when the spam bot was hitting a few months ago. Should it be cleaned up? Sure, but let's not rush to do it for the sake of doing it
If anything, it would be nice to start by looking at the feasibility of utilizing the message cache that already exists rather than making another one
green oriole
I didn't said it do "way too many API calls" I said it do "way too many API calls that it needs to" ๐
woeful thorn
Yeah, I get it
But there are plenty of things that need doing, and refactoring something that isn't broken may not be the most important thing to get done right now
green oriole
Yep
woeful thorn
You should, however, open an issue so we don't forget in an hour
green oriole
(apparently some tests needs to be written)
Done โ
https://github.com/python-discord/bot/issues/549 I'm working on this issue and I have different ideas on how to solve it but I don't know what's the best: should I just compare the filename, the file size (in bytes) and the size (in pixels) if it's an image, or should I download the file in a buffer and compare them?
green oriole
Hi, @gusty sonnet concerning https://github.com/python-discord/bot/issues/549, the mute was issued because of the duplicate of message, but if this rule haven't trigger, the attachment rule would have trigger anyway, and my testing shown that attachment trigger first. What do you want me to do?
hollow lichen
We currently see side effects of using different on_message handlers because the order is not guaranteed, so the "codeblock" formatter may duplicate a message that's going to be deleted by the token remover or a filter
There is a PR to solve that >.>
hardy gorge
I know, there are a lot of different on_message handlers (antispam, filters, codeblock formatter, token deleter, d.py command processor) for which an order may be desirable
hollow lichen
yep, basically almost one in each cog
hardy gorge
Yes, but for some, I don't see the issue in having them truly asynchronous with no predetermined order
For others, we should probably make sure we do the filtering/antispam/token detection first
Anyway, it would be a significant change that would need to be discussed extensively
green oriole
Pydis bravo is now setup right? So this should be archived? https://github.com/python-discord/bot-verification
hardy gorge
No
crude gyro
bravo is running, but there's basically nothing on it.
green oriole
You are running an empty server?
tawdry vapor
Well, they are given to us by the linode sponsorship after all
hardy gorge
Even if bravo is set-up, we'll still need to be able to spin up a bot to handle verification if @stable mountain goes down completely, even if it means running it from someone's home connection
crude gyro
it's not completely empty but our monitoring and emergency systems aren't live yet, they're still in the planning stage. we've been focused on getting everything up on alpha first.
hardy gorge
I see no reason to archive our back-up bot code
crude gyro
oh, I disagree
we should definitely archive it, we should be spinning up a real bot instead when the bot is down.
so that we still have moderation tools
green oriole
Lemon opened an issue saying you will archive it once bravo is up :)
hardy gorge
We won't have moderation tools if Linode goes down
crude gyro
sure, but alpha and bravo are in two different server parks.
hardy gorge
Unless we set-up a local site and some way of syncing it
crude gyro
two different countries.
both servers going down at the same time is incredibly unlikely
hardy gorge
how are we going to route the second server?
We had that idea of a mod bot, which is not a full bot
with a temp db (sqlite or something)
crude gyro
that design isn't fully decided on yet, but the general idea is to store the infractions on bravo temporarily and then sync them over to alpha once it goes live again
with, yeah, something like sqlite
hardy gorge
So, we'd need a specialized bot anyway
hardy gorge
Or do you want to make an alternative API and communicate internally?
crude gyro
I'd prefer to have the single bot just have an emergency mode, basically
because then all our changes to the main bot will always be present in the emergency bot
without having to keep the two repos manually in sync
hardy gorge
That makes sense
green oriole
And what about git branching? :)
crude gyro
?
green oriole
Just make a special emergency branch
crude gyro
no no no.
green oriole
That the point of git
crude gyro
that's absolutely not the point of git.
branches that don't get merged back into master are an anti-pattern.
with very few exceptions
this will not be one of them.
green oriole
Okay okay
crude gyro
if you want to have, basically, a second version of some repo that will be slightly different, the correct solution is a fork
not a branch.
green oriole
Talking about forks, why you have a clone of python.org and django-wiki on pydis github ?
tawdry vapor
The former was to add ourselves to the page of communities
the latter was because we needed to patch the wiki to support django 2.2
crude gyro
the former is probably not useful anymore and could be removed
but I think we still need the latter, sadly
green oriole
Guys can you try to run pipenv run lint in site? I get "flake8-bandit" failed during execution due to "'charmap' codec can't decode byte 0x9d in position 3487: character maps to <undefined>" Run flake8 with greater verbosity to see more details
glass pecan
are you on windows
green oriole
Yep
glass pecan
are you using cmd prompt
green oriole
yep
glass pecan
can you try using powershell instead
green oriole
no? ๐ Yes I try
molten bough
glad I'm not the only one
you're using pycharm though, aren't you? Why not just run through the terminal tab at the bottom?
green oriole
Because... I always forgot about that one
molten bough
I would suggest you look at the encoding of the files you've written
You can see it in the bottom toolbar when you have a file open in pycharm
UTF-8 is what you want
glass pecan
cmd prompt doesn't support unicode characters on output as a note, at least not by default
green oriole
oh does linting also lint stashed file?
molten bough
it might tell you where the problem is
molten bough
It certainly is.
molten bough
Of course you do
that was the point
you're trying to figure out what file is causing this problem
green oriole
There is at least 10000 lines of output
molten bough
Yep
green oriole
I'm going to try the ctrl f
molten bough
Yeeep
green oriole
If I don't explode the hard drive
I did pipenv run flake8 -vv > lint.log but I hasn't redirected everything..
Okay it work with 2>&1
Okay got it!
In \pydis_site\apps\api\tests\test_off_topic_channel_names.py
The file encoding is still UTF-8
Oh that's the end of the file
molten bough
What's it running?
green oriole
OS?
molten bough
What's the output if you print sys.getdefaultencoding()?
green oriole
utf-8
green oriole
yep
green oriole
The linting seem to have failed only on that one
molten bough
so, okay
here's what I found
I looked up a utf-8 table and the 9d there indeed doesn't appear to map to anything
it doesn't work with the byte before or byte after
yep
I see the problem
@tawdry vapor I believe this one is yours
amusingly I can't copy them
pycharm (or my os) is translating them to the correct utf-8 representation
crude gyro
sick of these windows encoding problems.
molten bough
actually I think windows is right in this case
there is a character here that is invalid utf-8
I'm not sure which one it is precisely
crude gyro
okay. I'm not sure how helpful that is
how can we solve it and why is it only happening on windows
molten bough
It should be converted to unicode escapes
crude gyro
unicode escapes are hideous and I hate that solution
if at all possible, this should be avoided.
molten bough
then someone needs to figure out which character is wrong, I suppose
I'll see if I can find anything in the debugger
crude gyro
I'm not exactly sure what you even mean by that
crude gyro
well that questionmark is valid unicode
stable mountainBOT
hardy gorge
That question mark appears twice in the file, does only one of them appear as invalid unicode?
crude gyro
so this just strikes me as a "oh, so windows can't handle fullwidth" problem.
stable mountainBOT
hardy gorge
Can you check the representation in the off_topic_names cog of bot?
that one also has the character
molten bough
ah, hang on
the byte in question is a 0x9D
which is.. 157?
yeah
but that byte isn't actually in this string
so where is that byte
@green oriole are you sure your branch is up to date?
green oriole
Yep
crude gyro
what if you removed the offending question mark, added it back again, and checked if the hex read the same way
green oriole
I just cloned it
crude gyro
does the 9D disappear when the questionmark is removed? are all such questionmarks going to create a 9d? etcetera. we're operating under a whole lot of assumptions here
not sure diving into this with a hex editor is even the right approach. there may be a higher level problem at play that we're missing, blabla forest for the trees.
molten bough
I'm just looking at the hex here
utf-8 has two-byte characters, if I'm not mistaken?
No, it can be four, can't it
crude gyro
can be and usually is
I think everything outside of ascii and latin-1 is
more or less.
molten bough
well okay, if we take a look at that string in a hex editor
the 27 is the apostrophe
F0 on its own is LATIN SMALL LETTER ETH
which is not our A
F09D is not a valid unicode character
9D is defined as OPERATING SYSTEM COMMAND
hardy gorge
The F09D combination is repeated later on
molten bough
it is, yeah
96 isn't our character
96A0 isn't valid unicode
so something is going on here for sure
molten bough
looking at encoding issues that akarys is having
ftr the test passes fine for me on linux
alright
so
0xF0 0x9D 0x96 0xA0 does indeed map to MATHEMATICAL SANS-SERIF CAPITAL A
weirdly that's not the actual representation in a string
that's what threw me off
the unicode escape is \U0001D5A0
alright, okay, this does seem alright having done a bit more research
the probable best fix here is to replace with escapes
I kind of agree with lemon in that escapes are ugly but on the other hand we shouldn't let file encoding interfere with unit tests
that's my two cents anyway
I suspect that flake8-bandit is opening files without specifying an encoding
molten bough
Looks like they're not gonna do it, yeah
this might be an issue in the upstream
pycodestyle
actually we're on flake8-bandit 1.0.2 aren't we
latest is 2.1.2
crude gyro
nice find, @hardy gorge
molten bough
the problem with that issue is that flake8-bandit no longer opens the file
it seems to defer to pycodestyle
okay, so maybe this needs testing with a newer bandit
crude gyro
maybe
crude gyro
I'd be open to updating it if that would solve it
tranquil topaz
flake8-bandit is that a CLI tool?
molten bough
it's a flake8 plugin that wraps the bandit tool
I swear to god I'm gonna make an emote of pipenv's loading spinner
haha
linting fine on linux with flake8-bandit ~=2.1
@green oriole can you pipenv install flake8-bandit~=2.1 and then pipenv run flake8 after, see what happens
crude gyro
that's good
green oriole
I suppose it passed
crude gyro
that means it's working
crude gyro
okay well then I guess we should bump that version
I don't really see any problem with that, it's only a dev tool and you've just tested that it works alright. it's a major bump but it's not like we're using it directly anyway, it's just running through flake8
and if it breaks something, it would just break at CI, it's not used in production at all
so if one of you wants to open a PR to bump it I'd be happy to merge it.
molten bough
@green oriole Do you want this one, or shall I do it?
green oriole
Yeah I would like to do it, I never done anything with pipenv :)
molten bough
Right on
crude gyro
any objections to this line of reasoning @hardy gorge @patent pivot @glass pecan @tranquil topaz ?
molten bough
I mean you've basically already done it, it's just the Pipfile and Pipfile.lock that are to be pushed up, assuming the staff OKs it
patent pivot
no objections
tranquil topaz
no objections
glass pecan
no objections
molten bough
no obje- wait
crude gyro
might even be okay to just include that version bump as kaizen in whatever it is you're already doing @green oriole
then you don't even have to make a new PR
molten bough
What is kaizen, exactly? I've seen you use that word a few times
crude gyro
a small, unrelated improvement
molten bough
Ah okay
woeful thorn
green oriole
Well
crude gyro
never mind then @green oriole
I'm just gonna get this merged instead
as usual, @woeful thorn is one jump ahead of the breadline
one swing ahead of the sword! he steals only what he can't afford!
god dammit now I have that song stuck in my head, thanks obama
crude gyro
merged. @green oriole you can just pull it from upstream now.
another mystery solved!
crude gyro
thanks @molten bough, @hardy gorge, @woeful thorn and the rest of the mystery gang.
molten bough
\o/
crude gyro
green oriole
Your link is broken :)
molten bough
It's nooot
green oriole
Oh no it worked the second time
hollow lichen
Hello, maybe it's more relevant in #community-meta, but anyway, does sometimes reviewing PR (especially from non-expert user) is more job than actually resolving the issue? I'm sometimes feeling a bit guilty to see long-review for short fix.
glass pecan
the question doesn't really suit meta, since it's not specifically about our community.
asking here isn't too bad if you're just asking in the context of our own projects, since this channel is about contributing to our own github repositories.
if you just wanted a general answer though, it might better suit #tools-and-devops
regarding us specifically, sure we get the occasional situation where a review can outweigh the effort of implementing it yourself
it's rare though
green oriole
Since we are more a learning community I think that's okay, if it was in a work environment that would be pretty problematic imo
glass pecan
yeah, that's it, we use the review process often to guide and improve
molten bough
How does staff feel about contribs reviewing PRs? We do have the access
glass pecan
reviews are welcome
merges are not allowed until at least one core dev has reviewed though
at a later date we'll be enforcing that with a check anyway
molten bough
Yep, we can't hit the button I think
hollow lichen
Yeah I was talking specifically about Pydis project
green oriole
Contribs can't merge anyway in master. It's a protected branch
mellow hare
Oh yeah I'm totally cool with anyone reviewing. Not like we're not going to eyeball the reviews anyway
glass pecan
we know
but to a core dev it shows as merge ready
and if they don't check the two reviews present they might accidentally assume it's all g
glass pecan
lol
molten bough
haha
mellow hare
As do slaps
molten bough
glass pecan
but yes, we'll happily accept at least one noncore review in place
there's nothing holding you guys back from that
mellow hare
Yeah, minimum one core review. I'd be happy to have however many contrib reivews
We've got a big ol' backlog, so any assistance you guys can offer is appreciated
glass pecan
the more reviews done before a core dev gets to it, often the less changes to apply at that point
so it's definitely useful
the dream is to do a review, say LGTM, merge it
without changes being asked for
mellow hare
And the more time I have to slack off review other ones
glass pecan
lol
i feel like we did some guideline thing about reviews
might be thinking of old discussions
green oriole
BTW when someone leave review comments, who should close them? The guy who opened the PR or the one that left the review?
glass pecan
you mean resolving?
green oriole
Oh yeah
glass pecan
the PR author can only resolve a review comment if it's been actually addressed as suggested
this way the comments that aren't resolved are all easy to see and discussions can continue
if there's two contribs that have a conflict in opinion, discussions can occur to resolve opinions, or a core dev can point out the preferred solution to go with
if the solution to that review comment means no code changed, the core dev will resolve it themselves
molten bough
Really it's going to depend on context
mellow hare
Like most things
molten bough
Just like most things i.. Yep
green oriole
Okay so if you address the comment no needs to wait for the comment author to close it
molten bough
If it's actually a comment on a specific line it'll be resolved when you change the line anyway