Adding autocodeblocking by hargoniX · Pull Request #15 · python-discord/bot

hargoniX · 2018-02-25T21:44:35Z

Autodetecting python code via checking if

it has at least three lines
is not throwing syntax errors when parsed with ast.parse
is not only ast.Expr

better type annotations

jb3 · 2018-02-25T21:54:43Z

+        """
+        Auto codeblock python code
+        """
+        if msg.content.count("\n") >= 3:  # more than three lines


Didn't we agree that it was over 3?

Three or more counts of \n should mean over 3 lines, unless the user includes trailing newlines.

Ahh yeah, my bad

jb3 · 2018-02-25T21:58:36Z


+    async def on_message(self, msg: Message):
+        """
+        Auto codeblock python code


Is this docstring necessary? In future it may not just be used for parsing Python code.

Yes, one could, of course, delete it but just for current usage. Depends on you del, not del?

If you look at the other files where event listeners are defined they don't have docstrings, to stay compliant, this should be removed.

jb3 · 2018-02-25T21:59:57Z

+            try:
+                tree = ast.parse(msg.content)  # no syntax errors
+                if not all(isinstance(node, ast.Expr) for node in tree.body):  # we dont want hi\nthere\nguys\nD
+                    await self.bot.send_message(msg.author, "```Python\n{}\n```".format(msg.content))


Should technically be a lower case p, as well as this we should probably either delete the message, or, have it send the current text in the codeblock tag.

Regarding previous comment, the lowercase p was referring to the syntax block.

"```python\n..."

We use f-strings all over the bot, this should also be an f-string.

jerbob · 2018-02-25T22:01:38Z

+        if msg.content.count("\n") >= 3:  # more than three lines
+            try:
+                tree = ast.parse(msg.content)  # no syntax errors
+                if not all(isinstance(node, ast.Expr) for node in tree.body):  # we dont want hi\nthere\nguys\nD


This inline comment seems unnecessary, maybe have a comment underneath describing the check more clearly?

Inline comments never really look nice, and a couple here are probably un-needed, especially after the if statement and the tree definition. Moving them above or underneath a line, and adding newlines around that is a better idea.

Your comments should follow PEP8 - they should start with a capital letter, and need to be grammatically correct. That means you're gonna need to use some punctuation. This comment, for example, deseperately needs a period and a comma.

jerbob · 2018-02-25T22:15:27Z

+        if msg.content.count("\n") >= 3:  # more than three lines
+            try:
+                tree = ast.parse(msg.content)
+                # tries to parse the code into a valid ast if it throws a SyntaxError it is no valid python


May I suggest # Attempts to parse the message into an AST node.
# Invalid Python code will raise a SyntaxError.

jerbob · 2018-02-25T22:16:59Z

+                tree = ast.parse(msg.content)
+                # Attempts to parse the message into an AST node.
+                # Invalid Python code will raise a SyntaxError.
+                if not all(isinstance(node, ast.Expr) for node in tree.body):  # we dont want hi\nthere\nguys\nD


This inline comment still bugs me. Try moving it to a new line and changing the comment to be more descriptive?

jerbob · 2018-02-25T22:22:36Z

+                # Attempts to parse the message into an AST node.
+                # Invalid Python code will raise a SyntaxError.
+                # => text which is not Python code will be filtered.
+                if not all(isinstance(node, ast.Expr) for node in tree.body):  # we dont want hi\nthere\nguys\nD


# we dont want hi\nthere\nguys\nD is what I meant. Try removing it and adding a new comment underneath

Something like:
# Single-line words are interpreted as expressions.
# This checks if all nodes were parsed as expressions.

swedgwood

I don't know much about AST so I commented on what I can, but that send_message() needs to change

swedgwood · 2018-02-25T22:05:47Z


        await ctx.invoke(self.info)

+    async def on_message(self, msg: Message):


Is having this directly in bot.py a good idea? It might be better to have it elsewhere and be imported, for readability. ( And have that docstring moved there )
But thats open to discussion.

I think it's fine for now.

swedgwood · 2018-02-25T22:18:49Z

+                # Attempts to parse the message into an AST node.
+                # Invalid Python code will raise a SyntaxError.
+                if not all(isinstance(node, ast.Expr) for node in tree.body):  # we dont want hi\nthere\nguys\nD
+                    await self.bot.send_message(msg.author, "```python\n{}\n```".format(msg.content))


send_message() is pre-rewrite, and I'm pretty sure you want to send it to the channel, so the rewrite version would be msg.channel.send(<message here>)
Also if the message (with the unformatted python code) were >1986 characters this line would throw an error, maybe add a check against this?

lemonsaurus

minor readability and consistency changes required.

lemonsaurus · 2018-02-25T22:22:25Z

+                # Attempts to parse the message into an AST node.
+                # Invalid Python code will raise a SyntaxError.
+                # => text which is not Python code will be filtered.
+                if not all(isinstance(node, ast.Expr) for node in tree.body):  # we dont want hi\nthere\nguys\nD


is this inline comment still necessary?

lemonsaurus · 2018-02-25T22:22:38Z

+                # Invalid Python code will raise a SyntaxError.
+                # => text which is not Python code will be filtered.
+                if not all(isinstance(node, ast.Expr) for node in tree.body):  # we dont want hi\nthere\nguys\nD
+                    await self.bot.send_message(msg.author, "```python\n{}\n```".format(msg.content))


this should be an f-string to be consistent with the rest of the repo.

lemonsaurus · 2018-02-25T22:23:04Z

        await ctx.invoke(self.info)

+    async def on_message(self, msg: Message):
+        if msg.content.count("\n") >= 3:  # more than three lines


this inline comment states the obvious, which PEP8 specifically says not to do.

jerbob · 2018-02-25T22:28:13Z

+                # Invalid Python code will raise a SyntaxError.
+                if not all(isinstance(node, ast.Expr) for node in tree.body):
+                    # we dont want multiple lines of single words like:
+                    '''


Is a docstring really necessary here?

How else would one do multiline comments without
#
#
#
or should i use that?

You don't really need an example. I think just explaining what the check does should suffice.

yeah don't use docstrings like this. if you did need a comment of that many lines, you should have used multiple # comments, but I agree with Ap that you don't really need that in this case. Just say it with words.

marked as outdated, reposting so it isnt marked as outdated

jerbob · 2018-02-25T22:29:36Z

+                    # that would be syntactically valid Python but in this case
+                    # just some random multiline text someone is sending.
+
+                    await msg.channel.send("```python\n{}\n```".format(msg.content))


This line still needs an f-string...

swedgwood · 2018-02-25T22:31:34Z

+                    # that would be syntactically valid Python but in this case
+                    # just some random multiline text someone is sending.
+
+                    await msg.channel.send("```python\n{}\n```".format(msg.content))


If msg.content were >1986 characters long, discord will get angry at you as the message exceeds 2000 characters, to avoid unnecessary errors I suggest some sort of check against this? Maybe format the string, then check if it is over 2000 characters before sending and deleting (Oh yeah also use f-strings, consistency ftw)

swedgwood · 2018-02-25T22:36:10Z

-                    await msg.channel.send(f"```python\n{msg.content}\n```")
-                    await self.bot.delete_message(msg)
+                    formatted = f"```python\n{msg.content}\n```"
+                    if len(formatted) > 2000:


Less than or equal to 2000, not more

lemonsaurus · 2018-02-26T00:18:34Z

    def __init__(self, bot: AutoShardedBot):
        self.bot = bot
+        self.code_block_channels = {303906576991780866: 0, 303906556754395136: 0, 303906514266226689: 0, 267624335836053506: 0}
+        # Stores allowed channels plus unix timestamp from last call


This comment is still in the wrong place. Comments should not be placed after the line they document.

lemonsaurus · 2018-02-26T00:18:49Z


    def __init__(self, bot: AutoShardedBot):
        self.bot = bot
+        self.code_block_channels = {303906576991780866: 0, 303906556754395136: 0, 303906514266226689: 0, 267624335836053506: 0}


I still want this line multilined.

I agree. This line is way too long.

jerbob · 2018-02-26T00:20:02Z

+                        # Invalid Python code will raise a SyntaxError.
+                        if not all(isinstance(node, ast.Expr) for node in tree.body):
+
+                            # We don't want multiple lines of single words,


Can I suggest having:
# Multiple lines of single words could be interpreted as expressions.
# This check is to avoid all nodes being parsed as expressions.
# (e.g. words over multiple lines)

jerbob · 2018-02-26T00:24:14Z

+                            # Multiple lines of single words could be interpreted as expressions.
+                            # This check is to avoid all nodes being parsed as expressions.
+                            # (e.g. words over multiple lines)
+                            howto = ("Please use syntax highlighted blocks, as it makes it more legible for other users.\n"


This string should be split over two lines. It exceeds 120 characters. Also made a minor edit to the sentence :D

"Please use syntax highlighted blocks, as it makes" "your code more legible for other users.\n"

jerbob · 2018-02-26T00:32:39Z

+                                    303906556754395136: 0,
+                                    303906514266226689: 0,
+                                    267624335836053506: 0
+        }


Try moving this closing parenthesis to right after the 0.
Reason: Travis

jerbob · 2018-02-26T00:47:13Z

 from dulwich.repo import Repo

-from bot.constants import PYTHON_GUILD, VERIFIED_ROLE
+from bot.constants import HELP1_CHANNEL, HELP2_CHANNEL, HELP3_CHANNEL, PYTHON_CHANNEL, PYTHON_GUILD, VERIFIED_ROLE


You know what to do.

Either have two imports on each line, or have three on each line. It'll make things look nicer, promise!

hmmmmmmmmmmmmmmmmmmmmmmmmm

Can we have an equal number of imports spread out over either 2 or 3 lines.
Currently, the line looks too long for use in a production setting.

jerbob · 2018-02-26T00:52:24Z

+HELP1_CHANNEL = 303906576991780866
+HELP2_CHANNEL = 303906556754395136
+HELP3_CHANNEL = 303906514266226689
+PYTHON_CHANNEL = 267624335836053506


Can we change this order to:

BOT_CHANNEL HELP1_CHANNEL HELP2_CHANNEL HELP3_CHANNEL PYTHON_CHANNEL DEVLOG_CHANNEL VERIFICATION_CHANNEL

To keep line lengths ascending, thanks

jerbob · 2018-02-26T00:57:57Z

 from dulwich.repo import Repo

-from bot.constants import PYTHON_GUILD, VERIFIED_ROLE
+from bot.constants import HELP1_CHANNEL, HELP2_CHANNEL, HELP3_CHANNEL, PYTHON_CHANNEL, PYTHON_GUILD, VERIFIED_ROLE


Can we have an equal number of imports spread out over either 2 or 3 lines.
Currently, the line looks too long for use in a production setting.

lemonsaurus · 2018-02-26T01:00:16Z

+                                     "```\n"
+                                     "print(\"Hello world!\")"
+                                     "```"
+                                     )  # noqa. E124


consistent end bracket indentation please. this should be on howto's level.

jerbob · 2018-02-26T01:00:38Z


-from bot.constants import PYTHON_GUILD, VERIFIED_ROLE
+from bot.constants import HELP1_CHANNEL, HELP2_CHANNEL, HELP3_CHANNEL
+from bot.constants import PYTHON_CHANNEL, PYTHON_GUILD, VERIFIED_ROLE


There's no need to have two import statements. You can spread the imports over 2 or 3 lines without repeating import statements, by using surrounding parentheses.

this I don't agree with. multiple imports are fine.

understandable, have a nice day

lemonsaurus

this will not do, sir.

lemonsaurus · 2018-02-26T01:00:44Z

 DEVLOG_CHANNEL = 409308876241108992
 VERIFICATION_CHANNEL = 352442727016693763

+


why is this here

This line is unacceptable. Requested changes.

jerbob · 2018-02-26T01:03:54Z

 DEVLOG_CHANNEL = 409308876241108992
 VERIFICATION_CHANNEL = 352442727016693763

+


This line is unacceptable. Requested changes.

jerbob

Nicely done. This is a great first contribution.

lemonsaurus · 2018-02-26T01:08:49Z

We've murdered this PR and you put up with it. Well done. The code looks great now.

hargoniX · 2018-02-26T01:10:18Z

hurray

hargoniX added 4 commits February 25, 2018 22:36

autocodeblocking

85a58fa

ast fix

0345760

better type annotations

ci there you go

962f8d0

line too long ==> remove comment

814bc34

jb3 reviewed Feb 25, 2018

View reviewed changes

jerbob reviewed Feb 25, 2018

View reviewed changes

hargoniX added 3 commits February 25, 2018 23:02

deleting message + lowercase python

7270b0c

comments

21f4b31

remove docstrings

24540be

jerbob reviewed Feb 25, 2018

View reviewed changes

you may

b0fd312

jerbob reviewed Feb 25, 2018

View reviewed changes

hargoniX added 2 commits February 25, 2018 23:18

not sure what you mean with move to a new line maybe this?

4382be1

descriptive enough?

86b80c6

jerbob reviewed Feb 25, 2018

View reviewed changes

swedgwood previously requested changes Feb 25, 2018

View reviewed changes

lemonsaurus requested changes Feb 25, 2018

View reviewed changes

better explanation of text filtering

872d040

jerbob reviewed Feb 25, 2018

View reviewed changes

rewrite

4e7f9a8

jerbob suggested changes Feb 25, 2018

View reviewed changes

fstring

04ed6c5

swedgwood suggested changes Feb 25, 2018

View reviewed changes

hargoniX added 3 commits February 25, 2018 23:32

pep8

b5217c1

better comment

d952ef0

discord message length

880c652

swedgwood suggested changes Feb 25, 2018

View reviewed changes

name error..... damn linter plugins

6530fc6

lemonsaurus requested changes Feb 26, 2018

View reviewed changes