snekbox wrapper
ChrisLovering opened this issue · 8 comments
This should include:
- Paste service utilities
- Base class for use with eval-like cogs (snekbox, internal eval, latex)
@ChrisLovering We can use all the methods present in site's APIClient without having to rewrite the code in the new Snekbox one.
We will override the ctor and url_for for it to use the new class instance that holds the base url.
The only thing that bugs me is that it'll be coupled to APIClient, but i don't think that client will change in the future which makes me think it's fine, what do you think ?
Update1
It could also be overkill, as we might not really all of that
Update2
Also, what do you think of moving the input sanitization to core ? I know it's not necessarily meant to be part of the client itself, but it could.
@ChrisLovering a couple of questions here
- Paste service utilities
I'm struggling to see why would snekbox wrapper contain paste service utilities, shouldn't it have its own wrapper ?
But also, are you talking about this precise utility ?
- Base class for use with eval-like cogs (snekbox, internal eval, latex)
I noticed that we don't use snekbox to eval code in the Internal Cog, is there a particular reason for that ? If not, wouldn't we want to migrate the Internal Cog's eval function to rely on snekbox ?
Last thing, I don't see any Latex cog in our code base, so I wanted to make sure whether I'm missing something or not.
@MarkKoz We could use your input here since I see you're the one who mentioned this in #85
I'm struggling to see why would snekbox wrapper contain paste service utilities, shouldn't it have its own wrapper ?
This was in the context of which modules should be ported over, so it doesn't mean that the paste utils have to be part of the snekbox wrapper.
Last thing, I don't see any Latex cog in our code base, so I wanted to make sure whether I'm missing something or not.
There used to be one in Sir Lancebot, but maybe it got removed or disabled due to issues.
I noticed that we don't use snekbox to eval code in the Internal Cog, is there a particular reason for that ? If not, wouldn't we want to migrate the Internal Cog's eval function to rely on snekbox ?
snekbox is for evaluating code in a sandboxed environment. Internal eval is for evaluating code within the context of the bot, so we do not want it sandboxed.
But also, are you talking about this precise utility ?
Yes, this https://github.com/python-discord/bot/blob/main/bot/utils/services.py
Alright, thanks @MarkKoz for clarifying.
In that case, here's what I'll do
- Start with the basic eval endpoint for snekbox
- Wait for Ionite's PR to finish, then port that over as part of the wrapper
- Open up a new issue for a new wrapper over the pasting service, and I'll take care of porting it as well (and discuss details in a separate issue)
In the meantime, I suggest editing the original comment that Chris has made about what should be ported over for better context to future readers
Though interal eval does not use snekbox, there is likely still some overlap in code. I imagine we can have a base class with common features, and we can also take the opportunity to make the eval commands behave more consistently with each other.
Internal eval is used in pretty much every bot, so we can just straight up port that command over I believe (modifying it to use the base class described above). Snekbox will only ever be in one bot, so I'm not sure why this issue is named "snekbox wrapper". @ChrisLovering can you clarify?
Yea, i did not imagine this to be a wrapper for the snekbox API itself, as it is already very trivial, but rather a wrapper for the logic involved in evaling code, as Mark says.
Re.
I had completely forgotten about this, up until @shenanigansd reminded me.
He also said that he'd want to take this up, so I'll be assigning him.