Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

snekbox wrapper #30

Open
ChrisLovering opened this issue Feb 21, 2022 · 8 comments
Open

snekbox wrapper #30

ChrisLovering opened this issue Feb 21, 2022 · 8 comments
Assignees

Comments

@ChrisLovering
Copy link
Member

ChrisLovering commented Feb 21, 2022

This should include:

  • Paste service utilities
  • Base class for use with eval-like cogs (snekbox, internal eval, latex)
@shtlrs
Copy link
Member

shtlrs commented Dec 12, 2022

I'll be taking care of this, discussed here

@shtlrs shtlrs self-assigned this Dec 12, 2022
@shtlrs
Copy link
Member

shtlrs commented Dec 13, 2022

@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.

@shtlrs
Copy link
Member

shtlrs commented Dec 17, 2022

@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

@MarkKoz
Copy link
Member

MarkKoz commented Dec 18, 2022

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

@shtlrs
Copy link
Member

shtlrs commented Dec 19, 2022

Alright, thanks @MarkKoz for clarifying.
In that case, here's what I'll do

  1. Start with the basic eval endpoint for snekbox
  2. Wait for Ionite's PR to finish, then port that over as part of the wrapper
  3. 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

@MarkKoz
Copy link
Member

MarkKoz commented Dec 19, 2022

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?

@ChrisLovering
Copy link
Member Author

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.

@shtlrs
Copy link
Member

shtlrs commented May 3, 2023

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.

@shtlrs shtlrs assigned shenanigansd and unassigned shtlrs May 3, 2023
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

No branches or pull requests

4 participants