python-discord / bot

The community bot for the Python Discord community
https://pythondiscord.com
MIT License
1.34k stars 663 forks source link

More snekbox commands (black/mypy/flake8 etc) #1880

Open wookie184 opened 2 years ago

wookie184 commented 2 years ago

Snekbox is pretty cool, we could use it to run a bunch of cool commands. https://github.com/python-discord/snekbox/pull/108 allows us to pass in custom arguments, so we should be able to do things like python -m black -c "code" fairly easily. An example of this is the timeit command https://github.com/python-discord/bot/pull/1602

We could use this to create a bunch of utility commands to run useful python command line tools (each tool would need to be tested to ensure it works under snekbox's limitations though).

Some ideas for things we could add:

One thing to be decided is if we would want to take input for options (e.g. line length for black), and if so how would we do that?

What are your thoughts on this, do we want any of these features? Anything else that hasn't been mentioned here? There are other tools like pyright or autopep8 we could add, although should we avoid adding too much bulk?

vcokltfre commented 2 years ago

Black

I think this would be great to have in snekbox, and with a specific command, so that we can make code readable when helping. That said, I'm not sure how widely useful it would be in the server (not that I'm saying we shouldn't add this at all, I personally would love this). Perhaps if we're adding black we should also add autopep8 to get a more to-PEP8 formatter for those who aren't fond of black's style.

MyPy

With the new type hinting channel I think this would be a great shortcut command to have so that we can demonstrate how static type checking works with various things in that channel. Definitely support this one.

Flake8

Also good for much the same reasons as black.

Dis

Boy have I wanted this one for a while. It's great to be able to show people the difference at a low level between 2 sets of code, which I've done several times, so having a shortcut to be able to do this with 1 command would be great, and would make me much more likely to show people that kinda info since it would be easier than having to do the whole eval flow.

Ast

Same as dis - useful for showing how code is parsed rather than executed.


Overall I think all of these would be cool additions to the bot. I would like to mention I think they should each be implemented on their own - dis is a command I'd really like to see and I'd hate for it to be held up as is common by a PR having a large number of components and thus taking a long time to review and merge, though I'm not sure this is totally viable since they'd likely be in the same file? (Maybe a good idea to do like the help channels cog and have multiple files, one for each major part)

One thing to be decided is if we would want to take input for options (e.g. line length for black), and if so how would we do that?

Now that we've moved to discord.py 2.0 we have access to discord.ext.commands.flags meaning we can have almost-UNIX-like command flags, so a possibility would be a command like:

!black --line-length 120 <code>

HassanAbouelela commented 2 years ago

All of these sound fine to add.

Maybe a good idea to do like the help channels cog and have multiple files, one for each major part

Most of the implementation here is made up of minor changes (snekbox: adding the module to the module list if it isn’t already there, testing. bot: any wrappers, call the API), it probably wouldn’t make sense to break it up into multiple PRs or files, as I imagine there will be overlap in a majority of the work.

vcokltfre commented 2 years ago

Gotcha, I guess it sounds like more stuff to do than it really is, but yeah should be fine after considering it more

onerandomusername commented 2 years ago

Now that we've moved to discord.py 2.0 we have access to discord.ext.commands.flags meaning we can have almost-UNIX-like command flags, so a possibility would be a command like:

!black --line-length 120 <code>

I do like the rest of it, but want to clarify, discord.ext.commands.flags are not unix-like.

The command would be more like this: !black line-length:120 code:'code to blackify'

to have an actual interface like that, we could use argparse on the command input.

MarkKoz commented 2 years ago

I'm not convinced the linters and type checkers make sense to add. There was a discussion about black here before https://discord.com/channels/267624335836053506/429409067623251969/851901038093140049

That said, I'm not sure how widely useful it would be in the server

That's a red flag to me. If there is no good justification, then it should not be done. Please let's not agree to adding features without solid reasoning. Maybe I'm the odd one out, but I am against adding features just for the sake of having them, to satisfy some impulses, to satisfy some really niche use cases, etc. Have a significant amount of users expressed a need for these tools? Are current alternatives, like the black and mypy playground websites, not sufficient? What advantages and disadvantages would there be to these commands over alternatives?

For these commands, users should feel inclined to execute the code blindly after typing it into Discord. If the code is being tested elsewhere first, then the purpose of the commands is already defeated. At that point, users may as well be copying the outputs of their local execution. Thus, we'd have to evaluate if the UX would be sufficient to encourage the "direct" usage of the command. I'm not sure if it would be any different from normal code execution, but I am bringing it up just in case.

dis and ast already have APIs that can be used, but I agree that having wrappers would be convenient. However, I'm concerned because these don't have CLI interfaces that can accept the code as an argument; the code can either be specified as a path to a file or piped in, neither of which snekbox supports currently. The input code could be modified and wrapped, but this may present other problems such as inaccurate line numbers and the potential to break out of the wrapper (which doesn't have any security implications, but just breaks the functionality). This may also apply to the first three tools, but I haven't looked into it.

HassanAbouelela commented 2 years ago

The original inspiration for this issue was a command to assist in typing conversations, and it was limited to mostly mypy.

The idea behind the other commands (at least ones like black which have the same -c interface as mypy) is that there shouldn't be much (if any) additional code required to make them run.

ichard26 commented 2 years ago

FWIW I don't think Black needs to be a first-class command especially as https://black.now.sh exists. I can't imagine it'll be used often enough to justify the (even if minor) maintenance cost. If someone wants to demo Black they could just point to a pre-filled Playground URL. I'd argue that adding Black as an available snekbox package would be more useful once we declare an official API for Black and start getting more user help requests in regards to that. I suspect this would be easier to maintain as well.

The only main advantage of having Black a command would be that helpees and helpers would be able to make their code generally readable more easily. Not sure how useful that'd be as I've gotten used to reading poorly formatted code :shrug: