Small grammar and code styles fixes. #145

DavidArchibald · 2018-12-24T02:50:41Z

I noticed some small grammar errors and code style problems. This fixes lots of small things, but the most common ones are:

Double quotes are turned into single quotes, because most of the codebase used them, but not all.
Every sentences' beginning is capitalized and end with a period, along with other small grammar fixes/word choices.
x in ['foo', 'bar'] is turned into y in ('foo', 'bar'), because tuples are immutable and don't have the(tiny) overhead lists do... basically just boils down to consistency and style.

This shouldn't change any actual functions of the script. If it does, it's a mistake.

itsthejoker · 2018-12-24T04:02:58Z

I'll work through this and take a look; the mixed " and ' are because I am slowly working on moving the codebase over to using the Black formatter, which defaults to " for readability purposes. The original codebase was written with ' because I prefer it, but " is more legible (especially in instances like asdf = '' vs asdf = "").

tor/core/admin_commands.py

itsthejoker · 2018-12-24T04:09:50Z

tor/core/admin_commands.py

-    :param reply: Object, the message object that contains the requested
-        command
-    :param config: the global config object
+    :param reply: Object, The message object that contains the requested


I'd rather this be ~~:param reply: The message object ...~~ :param reply: Object; the message...

itsthejoker · 2018-12-24T04:10:49Z

tor/core/admin_commands.py

@@ -44,38 +44,38 @@ def process_command(reply, config):
        except KeyError:
            if from_moderator(reply, config):
                reply.reply(
-                    "That command hasn't been implemented yet ):"
-                    "\n\nMessage a dev to make your dream come true."
+                    'That command hasn\'t been implemented yet ):'


Extraneous \ are the other reason that some of these have double quotes and some of them don't.

itsthejoker · 2018-12-24T04:11:33Z

tor/core/admin_commands.py

    if not from_moderator(reply, config):
        reply.reply(_(random.choice(config.no_gifs)))
        logging.info(
-            f'{reply.author.name} just tried to override. Lolno.'
+            f'{reply.author.name} just tried to override. Lol no.'


This particular character combination was deliberate. 😄

itsthejoker · 2018-12-24T04:14:51Z

tor/core/inbox.py

-    # Wrap each phrase in double-quotes (") and commas in between
-    phrases = '"' + '", "'.join(phrases) + '"'
+    # Wrap each phrase in double-quotes (") and commas in between.
+    phrases = f'"{", ".join(phrases)}"'


This does not do the same thing.

>>> phrases = ['asdf', 'qwer', 'erty'] >>> '"' + '", "'.join(phrases) + '"' '"asdf", "qwer", "erty"' >>> f'"{", ".join(phrases)}"' '"asdf, qwer, erty"' >>>

Additionally, it might work better to iterate in a different way:

phrases = ', '.join([f'"{phrase}"' for phrase in phrases])

This way we have wrapping of each phrase in the list in double-quotes, then join it by commas. Two separate actions.

itsthejoker · 2018-12-24T04:16:24Z

tor/core/user_interaction.py

@@ -38,8 +38,8 @@ def coc_accepted(post, config):
    """
    Verifies that the user is in the Redis set "accepted_CoC".

-    :param post: the Comment object containing the claim.
-    :param config: the global config dict.
+    :param post: The comment object containing the claim.


Comment here refers to the Comment class of PRAW.

itsthejoker · 2018-12-24T04:16:45Z

tor/core/user_interaction.py

@@ -76,14 +76,14 @@ def process_coc(post, config):
        ':fb-like:'
    ])

-    # Have they already been added? If 0, then just act like they said `claim`
+    # Have they already been added? If no, then just act like they said `claim`


0 is the literal return value.

itsthejoker · 2018-12-24T04:17:07Z

tor/core/user_interaction.py

@@ -95,8 +95,8 @@ def process_claim(post, config):
    Handles comment replies containing the word 'claim' and routes
    based on a basic decision tree.

-    :param post: The Comment object containing the claim.
-    :param config: the global config dict.
+    :param post: The comment object containing the claim.


References the Comment class from PRAW.

itsthejoker · 2018-12-24T04:23:32Z

tor/core/user_interaction.py

        # today
-        if top_parent.link_flair_text in ['', None]:
+        if top_parent.link_flair_text in ('', None):


According to SO, if this is done 10,000,000 times, switching to tuples will save us a grand total of... 0.005 seconds. My personal opinion is that it decreases clarity as well. I'm really not sure it's worth it; @TheLonelyGhost what do you think?

I didn't really mean for it to be a primarily speed based change, but stylistic. Regardless of the direction this ends up going, the usage of each should be made more consistent; while there are three x in [...] there is one instance of x in (...):

tor/tor/core/inbox.py

Line 192 in 3b8e9a0

elif item.subject in ('comment reply', 'post reply'):

I prefer this style, and so switched the rest to look like this.

itsthejoker · 2018-12-24T04:29:00Z

I've reviewed the particular pieces that I've found while reading through. There is one specific change in functionality that will need to be reverted, but the rest are stylistic. I'm not sure that I agree with a lot of the code comment changes simply because I perceive that a comment's job is to blend into the background and only be looked at if needed, which is why I don't capitalize a lot of them unless I want them to be seen. This is probably weird, but it's just the way I've been doing it over the years.

The changes from single quotes to double quotes are unnecessary as written above; there's no real need for that change to happen. If anything, we should be focusing on slowly redoing the rest of the codebase. As it stands, applying black wholesale across the codebase causes some issues; we still have to identify whether or not it can handle multi-line f-strings before we can really complete that process.

All that being said, there are some changes in here that I like - I'm not sure about the majority of them but the ones that increase clarity I'm all for.

itsthejoker · 2018-12-24T04:30:50Z

tor/core/users.py

@@ -39,9 +39,9 @@ def __init__(
        Create our own Redis connection if one is not passed in.
        We also assume that there is already a logging object created.

-        :param username: String; the username we're looking for. No fuzzing
+        :param username: String; The username we're looking for. No fuzzing


no letters after a semicolon should be capitalized.

itsthejoker

Github says I need to leave a comment when I request changes, so please see above comments. 😄

itsthejoker · 2019-01-15T19:45:00Z

After discussion with the core team, we've elected that these changes here don't quite match our preferences and style that we've decided to take with the codebase. We're also in the process of working out a lot of these kinks using Black, which will shift the bulk of formatting away from manual labor and also would overwrite a lot of these changes anyway. Thanks for submitting, but we won't be merging this one.

Small grammar and code styles fixes.

8d391de

DavidArchibald requested a review from a team as a code owner December 24, 2018 02:50

DavidArchibald mentioned this pull request Dec 24, 2018

Add Direct Message replies. #146

Merged

itsthejoker reviewed Dec 24, 2018

View reviewed changes

tor/core/admin_commands.py Show resolved Hide resolved

itsthejoker reviewed Dec 24, 2018

View reviewed changes

DavidArchibald mentioned this pull request Dec 24, 2018

RFC: Thoughts on Black? #127

Closed

itsthejoker reviewed Dec 24, 2018

View reviewed changes

itsthejoker suggested changes Dec 24, 2018

View reviewed changes

Fixes for review and other problems I noticed.

0e39530

itsthejoker closed this Jan 15, 2019

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Small grammar and code styles fixes. #145

Small grammar and code styles fixes. #145

DavidArchibald commented Dec 24, 2018

itsthejoker commented Dec 24, 2018

itsthejoker Dec 24, 2018 •

edited

Loading

itsthejoker Dec 24, 2018

itsthejoker Dec 24, 2018

itsthejoker Dec 24, 2018

TheLonelyGhost Jan 15, 2019

itsthejoker Dec 24, 2018

itsthejoker Dec 24, 2018

itsthejoker Dec 24, 2018

itsthejoker Dec 24, 2018

DavidArchibald Dec 24, 2018

itsthejoker commented Dec 24, 2018

itsthejoker Dec 24, 2018

itsthejoker left a comment

itsthejoker commented Jan 15, 2019

Small grammar and code styles fixes. #145

Small grammar and code styles fixes. #145

Conversation

DavidArchibald commented Dec 24, 2018

itsthejoker commented Dec 24, 2018

itsthejoker Dec 24, 2018 • edited Loading

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

itsthejoker commented Dec 24, 2018

Choose a reason for hiding this comment

itsthejoker left a comment

Choose a reason for hiding this comment

itsthejoker commented Jan 15, 2019

itsthejoker Dec 24, 2018 •

edited

Loading