Fancier IPython-like eval command by Martmists-GH · Pull Request #3 · python-discord/bot

Martmists-GH · 2018-02-09T13:44:19Z

Supports Embeds
Pretty-print
Shortens if too long
Supports catching stdout
Supports async code
Automatically awaits awaitables if they are the return value
Some other misc changes

- Supports Embeds - Pretty-prints - Shortens - Supports catching stdout

gdude2002

You've got a bunch of linting errors.

./bot/cogs/eval.py:12:1: F401 'discord.ext.commands.Context' imported but unused
./bot/cogs/eval.py:140:0: P101 format string does contain unindexed parameters
./bot/cogs/eval.py:143:1: B102 Use of exec detected.
./bot/cogs/eval.py:171:1: E303 too many blank lines (3)
./bot/cogs/eval.py:22:1: W293 blank line contains whitespace
./bot/cogs/eval.py:30:23: F821 undefined name 'io'
./bot/cogs/eval.py:33:1: W293 blank line contains whitespace
./bot/cogs/eval.py:4:1: F401 'io.StringIO' imported but unused
./bot/cogs/eval.py:5:1: I100 Import statements are in the wrong order. import inspect should be before from io
./bot/cogs/eval.py:65:23: F821 undefined name 'io'
./bot/cogs/eval.py:7:1: I100 Import statements are in the wrong order. import pprint should be before import textwrap
./bot/cogs/eval.py:9:1: I100 Import statements are in the wrong order. import re should be before import traceback

gdude2002 · 2018-02-09T13:47:45Z

  email:
    on_success: never
-    on_failure: never
+    on_failure: never


This file is out of scope of the PR, please remove it.

I don't even know where that line came from, probably my auto-travis-lint?

gdude2002 · 2018-02-09T13:51:51Z

-        elif code.startswith("`") and code.endswith("`"):
-            code = code[1:-1]
+                # Indent the 3 dots correctly
+                s = (f"{{:<{len(str(self.ln))+2}}}...: ").format("")


Can you comment this better? It's very unclear what this would do for a beginner

gdude2002 · 2018-02-09T13:53:14Z

+        _code = """
+async def func():
+    try:
+        with contextlib.redirect_stdout(self.stdout):


Will doing this mess up printing stuff to stdout from the rest of the bot while the eval is being processed?

Contextlib should handle that. I've personally never had such issue.

gdude2002 · 2018-02-09T14:00:08Z

+                # far enough to align them.
+                # we first `str()` the line number
+                # then we get the length
+                # and do a simple {:<LENGHT}


gdude2002 · 2018-02-09T14:00:39Z

This is from one of your other bots originally, right? What's the license on that bot?

gdude2002 · 2018-02-09T14:04:50Z

./bot/cogs/eval.py:57:18: W291 trailing whitespace
./bot/cogs/eval.py:58:30: W291 trailing whitespace
./bot/cogs/eval.py:6:1: I100 Import statements are in the wrong order. import pprint should be before from io
./bot/cogs/eval.py:61:18: W291 trailing whitespace

Martmists-GH · 2018-02-09T14:10:48Z

The license on said bot makes contributors unable to share it without permission, and I'm giving permission by adding it myself.

I will fix those issues in a second.

gdude2002 · 2018-02-09T14:14:33Z

Alright, that's fine.

Really the only other thing I'd like to address in this PR is readability - there's lots of overly-short variable names, one-liners and somewhat opaque string concatenations.

We'd prefer if you could deal with that, but we can otherwise try to do it ourselves.

lemonsaurus

The code is good, but readability is quite poor. I'd like to see the readability improved before we merge this. there are too many single-letter variable names, too many convoluted one-liners, too much nesting.. If we can get a version of this that follows the Zen of Python, I'd probably be willing to merge it.

lemonsaurus · 2018-02-09T13:51:23Z

-        elif code.startswith("`") and code.endswith("`"):
-            code = code[1:-1]
+                # Indent the 3 dots correctly
+                s = (f"{{:<{len(str(self.ln))+2}}}...: ").format("")


Poor readability.

lemonsaurus · 2018-02-09T13:52:45Z

+        if len(lines) != 1:
+            lines += [""]
+
+        # Create the inpit dialog


should it say input?

lemonsaurus · 2018-02-09T13:54:06Z

+        # Create the inpit dialog
+        for i, line in enumerate(lines):
+            if i == 0:
+                s = f"In [{self.ln}]: "


single letter variable names

lemonsaurus · 2018-02-09T13:58:03Z

+                if line.startswith("return"):
+                    line = line[6:].strip()
+
+            res += s + line + "\n"


convoluted string concatenation

Using f-strings or str.format here would seem unneeded to me.

if res and s had better variable names, it would probably be okay.

lemonsaurus

some more readability concerns.

lemonsaurus · 2018-02-10T08:48:03Z

+            func = self.env['func']
+            res = await func()
+
+        except:  # noqa pylint: disable=bare-except


would bare-except trigger if this was except Exception:? if not, isn't that better than disabling a pylint rule?

lemonsaurus · 2018-02-10T08:50:51Z

+            if pretty.count("\n") > 20:
+                # Text too long, shorten
+                li = pretty.split("\n")
+                pretty = "\n".join(li[:3]) + "\n ...\n" + "\n".join(li[-3:])


lemonsaurus · 2018-02-10T08:53:12Z

+            inp = inp[4:]

-        code = string.strip()
+        lines = [l for l in inp.split("\n") if l.strip()]


for this to be beginner friendly, listcomps should have an accompanying comment. the meaning of the if l.strip() in this context may not be immediately obvious.

lemonsaurus · 2018-02-10T08:54:55Z

+
        self.interpreter = Interpreter(bot)
+
+    def _format(self, inp, out):


this function needs a docstring so it will be obvious what exactly it's used for.

lemonsaurus · 2018-02-10T08:56:05Z

+                if line.startswith("return"):
+                    line = line[6:].strip()
+
+            res += s + line + "\n"


if res and s had better variable names, it would probably be okay.

lemonsaurus · 2018-02-10T08:56:51Z

+        if len(lines) != 1:
+            lines += [""]
+
+        # Create the inpit dialog


should it say input?

lemonsaurus · 2018-02-10T08:59:29Z

+
+            res += s + line + "\n"
+
+        self.stdout.seek(0)


maybe block comment this part to explain why you're doing it. generally speaking, the code could use many such comments explaining the flow of this method. Ideally we want people who are moderately new to python to be able to skim through this method and understand the flow without necessarily understanding all the code.

lemonsaurus · 2018-02-10T09:04:28Z

+            if (isinstance(out, str) and
+                    out.startswith("Traceback (most recent call last):\n")):
+                # Leave out the traceback message
+                out = "\n" + "\n".join(out.split("\n")[1:])


it's got string concatenation, a .split nested inside a .join, and a container slice, all in one line. it's too dense. it'd be better to split this into more lines if it can't be simplified.

lemonsaurus · 2018-02-10T09:10:13Z

+                out = "\n" + "\n".join(out.split("\n")[1:])
+
+            pretty = (pprint.pformat(out, compact=True, width=60)
+                      if not isinstance(out, str) else str(out))


readability suffers because of the ternary - instead of reading the logic in semantic order (if this then that), the reader has to parse it as (that if this). And why do we need to cast out to a str when the isinstance clearly establishes that it is already a str?

the far simpler

if not isinstance(out, str): pretty = pprint.pformat(out, compact=True, width=60) else: pretty = out

would be better.

lemonsaurus · 2018-02-10T09:21:46Z

for the record, I've left comments for absolutely all my readability concerns now. if they were all adressed, I'd approve.

gdude2002 · 2018-02-15T10:24:16Z

Has there been any movement on this?

Martmists-GH · 2018-02-15T11:36:52Z

Not yet, it's been a busy week so far. I'll probably push a fix tomorrow

Martmists-GH · 2018-02-16T20:00:03Z

@gdude2002 @1mn

lemonsaurus · 2018-02-16T20:05:41Z

That's a lot better. Thanks!

gdude2002 · 2018-02-16T20:06:17Z

This is a considerable improvement. Good work.

Merge pull request #1028 from dolphingarlic/master

This commit should be squashed upon PR merge. - Another fix in `windows-path.md`

Martmists-GH added 2 commits February 9, 2018 14:43

Fancier IPython-like eval command

1251d8d

- Supports Embeds - Pretty-prints - Shortens - Supports catching stdout

Cache pip to speed up travis in the future

040dad1

gdude2002 suggested changes Feb 9, 2018

View reviewed changes

Fix snekchek

be9b24d

gdude2002 reviewed Feb 9, 2018

View reviewed changes

Martmists-GH added 2 commits February 9, 2018 14:57

Update eval.py

bfab2ba

Re-order imports

06cfa25

gdude2002 reviewed Feb 9, 2018

View reviewed changes

gdude2002 self-assigned this Feb 9, 2018

gdude2002 added the t: feature New feature or request label Feb 9, 2018

lemonsaurus requested changes Feb 9, 2018

View reviewed changes

Martmists-GH added 2 commits February 10, 2018 09:53

Merge branch 'master' into patch-1

86cfe8a

Comply with mentioned issues

d2877fa

lemonsaurus requested changes Feb 10, 2018

View reviewed changes

Martmists-GH added 3 commits February 16, 2018 20:53

Fix travis

cbe5456

Conform to requested edits

95fdfe7

Fix travis again

ed556af

Martmists-GH added 2 commits February 16, 2018 21:00

Update eval.py

c1be1f4

Merge branch 'master' into patch-1

8cb5dfb

lemonsaurus approved these changes Feb 16, 2018

View reviewed changes

gdude2002 approved these changes Feb 16, 2018

View reviewed changes

gdude2002 merged commit 47136cf into python-discord:master Feb 16, 2018

Martmists-GH deleted the patch-1 branch February 16, 2018 20:07

ChrisLovering pushed a commit that referenced this pull request Apr 30, 2021

Merge pull request #3 from python-discord/main

82d4dd2

Merge pull request #1028 from dolphingarlic/master

TizzySaurus added a commit that referenced this pull request Sep 25, 2021

Bluenix Review #3

9b8927f

This commit should be squashed upon PR merge. - Another fix in `windows-path.md`

TizzySaurus added a commit that referenced this pull request Sep 25, 2021

Bluenix Review #3

093fdaa

This commit should be squashed upon PR merge. - Another fix in `windows-path.md`

TizzySaurus added a commit that referenced this pull request Oct 14, 2021

Bluenix Review #3

5c22de1

This commit should be squashed upon PR merge. - Another fix in `windows-path.md`


		self.interpreter = Interpreter(bot)

		def _format(self, inp, out):

Uh oh!

Conversation

Martmists-GH commented Feb 9, 2018

Uh oh!

gdude2002 left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

gdude2002 Feb 9, 2018 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

gdude2002 commented Feb 9, 2018

Uh oh!

gdude2002 commented Feb 9, 2018

Uh oh!

Martmists-GH commented Feb 9, 2018

Uh oh!

gdude2002 commented Feb 9, 2018

Uh oh!

lemonsaurus left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

lemonsaurus left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

lemonsaurus commented Feb 10, 2018

Uh oh!

gdude2002 commented Feb 15, 2018

Uh oh!

Martmists-GH commented Feb 15, 2018

Uh oh!

Martmists-GH commented Feb 16, 2018

Uh oh!

lemonsaurus commented Feb 16, 2018

Uh oh!

gdude2002 commented Feb 16, 2018

gdude2002 Feb 9, 2018 •

edited

Loading