[Discussion] Unify code style #67

jchristgit · 2018-05-02T20:21:53Z

So, I've went ahead and ran all Python files through black. My motivation behind this is that black enforces a consistent style throughout the project, and with a community project, I think that that's useful.

I'd be very interested for opinions towards this. The only thing I dislike is the wrapping to multiple lines of lines with long comments - but it's easy to fix this by simply putting the comment above, and it's not too common either. Looking forward to your input.

One more thing, it's also possible to use black in CI with black --check which returns a status code depending on whether or not something would've been changed, useful for not having to check style on PRs ourselves.

lemonsaurus

Most of these changes are clear improvements, in my opinion. It changes a bunch of ''s into "'s, which is great, and it unifies multiline stuff to a certain degree. however, I think it's a bit inconsistent about it.

I've left comments in the many places that made me go wtf, most of them are keyword args in an Embed constructor and occasionally just odd stuff that needs to be fixed by a human.

I think this PR might be worth doing, if we get some more reviews and make all the changes that need making - but considering on how many points I found myself disagreeing with this, I would not be in favor of adding this to CI, I think.

lemonsaurus · 2018-05-04T20:41:33Z

bot/cogs/bot.py

@@ -153,27 +169,37 @@ def codeblock_stripping(self, msg: str):
                        # This check is to avoid all nodes being parsed as expressions.
                        # (e.g. words over multiple lines)
                        if not all(isinstance(node, ast.Expr) for node in tree.body):
-                            codeblock_tag = await self.bot.get_cog("Tags").get_tag_data("codeblock")
+                            codeblock_tag = await self.bot.get_cog("Tags").get_tag_data(


this one strikes me as weird. not really a readability improvement.

lemonsaurus · 2018-05-04T20:42:40Z

bot/cogs/clickup.py

-    "Authorization": CLICKUP_KEY,
-    "Content-Type": "application/json"
-}
+HEADERS = {"Authorization": CLICKUP_KEY, "Content-Type": "application/json"}


also not sure how this is an improvement. seems inconsistent with the other changes.

lemonsaurus · 2018-05-04T20:42:56Z

bot/cogs/clickup.py

        else:
            # Save all the lists with their IDs so that we can get at them later
            for project in result["projects"]:
                for list_ in project["lists"]:
                    self.lists[list_["name"]] = list_["id"]
-                    self.lists[f"{project['name']}/{list_['name']}"] = list_["id"]  # Just in case we have duplicates
+                    self.lists[f"{project['name']}/{list_['name']}"] = list_[


this is also crazy

lemonsaurus · 2018-05-04T20:45:23Z

bot/cogs/clickup.py

            embed = Embed(
-                colour=Colour.red(),
-                description=f"`{result['ECODE']}`: {result['err']}"
+                colour=Colour.red(), description=f"`{result['ECODE']}`: {result['err']}"


?? this seems inconsistent with other changes.

lemonsaurus · 2018-05-04T20:45:42Z

bot/cogs/clickup.py


            for member in result["team"]["members"]:
                embed.add_field(
-                    name=member["user"]["username"],
-                    value=member["user"]["id"]
+                    name=member["user"]["username"], value=member["user"]["id"]


and again. maybe I just don't get it.

lemonsaurus · 2018-05-04T21:10:42Z

bot/cogs/math.py

-                "res": 300,
-                "color": 808080
-            }
+            data = {"latex": parsed, "res": 300, "color": 808080}


I also liked this better multilined.

lemonsaurus · 2018-05-04T21:10:51Z

bot/cogs/math.py


            async with self.bot.http_session.post(LATEX_URL, data=data) as resp:
                html = await resp.text()

-            name = search(r'hist\.request\.basename = "(?P<url>[^"]+)"', html).group('url')
+            name = search(r'hist\.request\.basename = "(?P<url>[^"]+)"', html).group(


lemonsaurus · 2018-05-04T21:12:21Z

bot/formatter.py


            # strip the command off bot. and ()
-            stripped_command = self.command.name.replace(HELP_PREFIX, "").replace("()", "")
+            stripped_command = self.command.name.replace(HELP_PREFIX, "").replace(


this is also super weird.

lemonsaurus · 2018-05-04T21:13:04Z

bot/pagination.py

@@ -74,7 +73,10 @@ def add_line(self, line='', *, empty=False):
            The line was too big for the current :attr:`max_size`.
        """
        if len(line) > self.max_size - len(self.prefix) - 2:
-            raise RuntimeError('Line exceeds maximum page size %s' % (self.max_size - len(self.prefix) - 2))
+            raise RuntimeError(
+                "Line exceeds maximum page size %s"


why are we using c-style % interpolation?

lemonsaurus · 2018-05-04T21:14:16Z

bot/pagination.py

@@ -129,26 +141,31 @@ def event_check(reaction_: Reaction, user_: Member):

            no_restrictions = (
                # Pagination is not restricted
-                not restrict_to_user or
+                not restrict_to_user
+                or
                # The reaction was by a whitelisted user


naw, this part doesn't work for me. it'd be better to have an inline comment, I think. or something else. just find any other solution. we can't have that lonely or

This reverts commit 25ab218.

lemonsaurus

there's.. a lot of things that I don't agree with. I made it about 3/4ths before I gave up. I just don't think this is good enough currently, it would take a whole lot of changes to make it acceptable to merge.

lemonsaurus · 2018-05-05T07:19:43Z

bot/cogs/bot.py

-    DEVTEST_CHANNEL, HELP1_CHANNEL, HELP2_CHANNEL,
-    HELP3_CHANNEL, HELP4_CHANNEL, MODERATOR_ROLE, OWNER_ROLE,
-    PYTHON_CHANNEL, PYTHON_GUILD, VERIFIED_ROLE
+    ADMIN_ROLE,


this is still kinda insane.

lemonsaurus · 2018-05-05T07:20:22Z

bot/cogs/bot.py

-            url="https://github.com/discord-python/bot",
-            icon_url=BOT_AVATAR_URL
-        )
+        embed.set_author(name="Python Bot", url="https://github.com/discord-python/bot", icon_url=BOT_AVATAR_URL)


I still prefer the old one here. whenever we list things like this, it's nice to multiline it.

lemonsaurus · 2018-05-05T07:21:28Z

bot/cogs/clickup.py

            )
+            embed = Embed(colour=Colour.red(), description=f"`{result['ECODE']}`: {result['err']}")


not an improvement

lemonsaurus · 2018-05-05T07:21:50Z

bot/cogs/clickup.py

-                    name=member["user"]["username"],
-                    value=member["user"]["id"]
-                )
+                embed.add_field(name=member["user"]["username"], value=member["user"]["id"])


prefer the old one.

lemonsaurus · 2018-05-05T07:23:08Z

bot/cogs/clickup.py

-                    name=f"{project['name']} ({project['id']})",
-                    value=lists
-                )
+                embed.add_field(name=f"{project['name']} ({project['id']})", value=lists)


I like this multilined.

lemonsaurus · 2018-05-05T07:32:12Z

bot/cogs/hiphopify.py

-                f"Error encountered when trying to unhiphopify {member.display_name}:\n"
-                f"{response}"
-            )
+            log.warning(f"Error encountered when trying to unhiphopify {member.display_name}:\n" f"{response}")


I prefer the way this used to be.

lemonsaurus · 2018-05-05T07:32:27Z

bot/cogs/math.py

+        env={},  # Disable environment variables for security
+        stdout=PIPE,
+        stderr=STDOUT,
+    )  # reroute all to stdout


comment in wrong place.

lemonsaurus · 2018-05-05T07:32:54Z

bot/cogs/math.py

-    for _ in range(timeout*4):  # Check if done every .25 seconds for `timeout` seconds
-        await asyncio.sleep(1/4)
+    proc = Popen(
+        [  # noqa: B603,S603


this probably breaks flake8, noqa is on the wrong line.

lemonsaurus · 2018-05-05T07:33:22Z

bot/cogs/tags.py

-    ADMIN_ROLE, BOT_COMMANDS_CHANNEL, DEVTEST_CHANNEL,
-    HELPERS_CHANNEL, MODERATOR_ROLE, OWNER_ROLE,
-    SITE_API_KEY, SITE_API_TAGS_URL, TAG_COOLDOWN
+    ADMIN_ROLE,


gdude doesn't love this

lemonsaurus · 2018-05-05T07:33:30Z

bot/cogs/tags.py

-    BOT_COMMANDS_CHANNEL,
-    HELPERS_CHANNEL
-)
+TEST_CHANNELS = (DEVTEST_CHANNEL, BOT_COMMANDS_CHANNEL, HELPERS_CHANNEL)


jchristgit · 2018-05-05T08:01:25Z

I agree that some of the changes are fairly weird, and having an automated program mess with things like bot/constants.py or long imports with from x import (y, z, a, b, c) is kinda bound for terror. Regarding multi lining, it probably only operates based on line length, sometimes resulting in weird things like

my_long_function_call('does').some_stuff(
    True
)

which just looks weird. I also agree with the comments being super weird sometimes. I'll close it for now, maybe we can find a better way to get a more consistent code style throughout the project at some point.

lemonsaurus · 2018-05-05T08:03:03Z

the idea of unifying code style is a great one. and I wouldn't mind if we enforced it strictly - but if we're going to do that, we need to absolutely 100% agree on what that code style should be. if we could find a tool sort of like Black but which allowed us to customize it to our liking ... but I doubt such a tool exists.

jchristgit added 2 commits May 2, 2018 22:18

Format source code with black.

25ab218

Fix flake8 complaints about subprocess.

b5be714

lemonsaurus requested changes May 4, 2018

View reviewed changes

lemonsaurus requested review from gdude2002 and jerbob May 4, 2018 21:19

jchristgit added 2 commits May 5, 2018 00:08

Revert "Format source code with black."

17a518f

This reverts commit 25ab218.

Rerun formatting with line length of 120.

f2cfd68

lemonsaurus requested changes May 5, 2018

View reviewed changes

jchristgit closed this May 5, 2018

jchristgit deleted the unified-code-formatting branch May 5, 2018 08:44

pydis-bot mentioned this pull request Nov 17, 2018

Django API: HTTP error handling #127

Closed

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

[Discussion] Unify code style #67

[Discussion] Unify code style #67

jchristgit commented May 2, 2018 •

edited

Loading

lemonsaurus left a comment

lemonsaurus May 4, 2018

lemonsaurus May 4, 2018

lemonsaurus May 4, 2018

lemonsaurus May 4, 2018

lemonsaurus May 4, 2018

lemonsaurus May 4, 2018

lemonsaurus May 4, 2018

lemonsaurus May 4, 2018

lemonsaurus May 4, 2018

lemonsaurus May 4, 2018

lemonsaurus left a comment

lemonsaurus May 5, 2018

lemonsaurus May 5, 2018

lemonsaurus May 5, 2018

lemonsaurus May 5, 2018

lemonsaurus May 5, 2018

lemonsaurus May 5, 2018

lemonsaurus May 5, 2018

lemonsaurus May 5, 2018

lemonsaurus May 5, 2018

lemonsaurus May 5, 2018

jchristgit commented May 5, 2018

lemonsaurus commented May 5, 2018

		)
		embed = Embed(colour=Colour.red(), description=f"`{result['ECODE']}`: {result['err']}")

[Discussion] Unify code style #67

[Discussion] Unify code style #67

Conversation

jchristgit commented May 2, 2018 • edited Loading

lemonsaurus left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

lemonsaurus left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

jchristgit commented May 5, 2018

lemonsaurus commented May 5, 2018

jchristgit commented May 2, 2018 •

edited

Loading