Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Suggestions Of Changes #3

Closed
RoboticMind opened this issue Aug 6, 2019 · 4 comments
Closed

Suggestions Of Changes #3

RoboticMind opened this issue Aug 6, 2019 · 4 comments

Comments

@RoboticMind
Copy link

This list isn't a complete list but it does include some really important changes.

Miscellaneous

  • Just have once license file. You don't need both LICENSE and LICENSE.txt. This should be easy enough to fix.

  • File names should be more clear. Mentions_bot and comment_bot and submissions_bot aren't clear names to differentiate. They probably should all be one file anyway with different threads. You can read more on how multithreading works in python here.

Code Style

Reading through the code, there's a number of things that should be addressed in terms of coding style.

  • very long one-line strings

in Python, you can actually make multi-line strings that are much easier to read.

cleaner_message = """This message is very long.
In this format, newlines are newlines in the code instead of \n.
so I can make a multi-line message actually be multiple lines in code instead of 
one long line"""

or you can do something like this if you still want it to be just on line

cleaner_message = ("this message is going to be way too long to put on one"
                  "line of code. In this method, cleaner_message will still be one line,"
                  "but I can write it out in more lines in code")
  • I'd recommend using PEP8 style guidlines (editors can actually verify that you are following the guidelines). Using a consistent styling type is important and PEP8 is a pretty widespread style so it's a good one to use.

    • Gives other people reading your code a reference point

    • Following PEP8 will actually have your editor warn you when your lines get to large. PEP8 styling has lines no larger than 79 characters.

    • Editors/IDEs nicely support PEP8


There's other feedback to give but I think that's feedback is at least good to start with

@KilledMufasa
Copy link
Owner

Let me start of by saying that I really appreciate you taking the time to write this detailed issue.

Just have once license file. You don't need both LICENSE and LICENSE.txt. This should be easy enough to fix.

You're correct. The file has now been removed.

File names should be more clear. Mentions_bot and comment_bot and submissions_bot aren't clear names to differentiate.

Do you have any suggestions? For me personally, the names work, but I can imagine them being confusing for people new to the code. How would you feel about something like mentions_stream.py?

They probably should all be one file anyway with different threads. You can read more on how multithreading works in python here.

Not gonna lie, I had no clue multithreading was a thing in Python. The coming days I will experiment with this. Thx for the suggestion!

in Python, you can actually make multi-line strings that are much easier to read.

Another thing I didn't know. As you probably figured by now, I am very new to Python so I'm learning as I go (nevertheless my apologies for the crappy code). Consider this done with the next commit.

I'd recommend using PEP8 style guidlines

Thx for that recommendation. I'll make sure the next commit accounts for these guidelines.

I haven't exactly made major changes since you opened this issue and I'm sorry about that. Once I have the time to program it all (which should be within a couple of days), I'll make a commit tagging you. Thank you!

KilledMufasa added a commit that referenced this issue Sep 1, 2019
…d filenames and more

This release (v1.4) brings a lot of under the hood fixes and changes. To make the code more readable, these files are now following a slightly altered form of pylint. Tabs have been changed to spaces. All debugging is now done using logging() instead of print(). The filenames have been changed to better represent their function. A lot of these changes were suggested by @RoboticMind who [opened this super-useful issue](#3).

All user-agents have been moved to the config file as suggested by @jellysnake in [Issue #4](#4).

And last but not least, some comments, the PM template and reply template have been altered slightly.
@KilledMufasa
Copy link
Owner

Hey again, sorry for the late update, just wanted to let you know that I've implemented most of your suggestions (in some form or another).

  • I removed the unnecessary license file earlier
  • The filenames have been changed to check_comments.py, check_submissions.py and check_mentions.py to better reflect their usage.
  • My code is now following (a slightly altered form of) the guidelines of Pylint. PEP8 is great, but imo too limiting.

Since the long string templates are full of markdown, a lot of special characters need to be escaped among other inconveniences. So the multi-line strings are a thing I want to do another time.

I've investigated whether or not multithreading would be possible. Turns out Praw (what I'm using for working with the Reddit API) is not thread safe. Their docs say the following:

The recommended way to run multiple instances of PRAW is to simply write separate independent python programs. With this approach one program can monitor a comment stream and reply as needed, and another program can monitor a submission stream, for example.

In a nutshell, instances of Reddit are not thread safe for a number of reasons in its own code and each instance depends on an instance of requests.Session, which is not thread safe.

There are some workarounds, but these are difficult and they defeat the purpose. It only gives us more headache. With this info (and a lot testing) in mind, I've decided not to implement multithreading until a better solution has been found.

Please don't think that I'm ignoring your advice, in fact, I've tried to implement all of your suggestions, but truth is I'm just not good enough at Python yet.

I'm closing this issue, but the suggestions you've made that haven't been implemented yet are on the to-do list. Thx again for your super-useful issue!

@RoboticMind
Copy link
Author

If it's not thread-safe, then it would make sense to keep it as separate programs to run. Probably just want to put that in the README then.

For the markdown strings, the character escapes shouldn't be any different from a one-line string. Maybe there's something else there that I'm not seeing?

@KilledMufasa
Copy link
Owner

KilledMufasa commented Sep 2, 2019

For the markdown strings, the character escapes shouldn't be any different from a one-line string.

You're correct. It's the exact same. I explained myself badly. Reddit's markdown is highly sensitive for errors. Every space, special character, bracket, line-break, it changes something. And to be perfectly honest with you, I don't want to have to deal with that right now. I change the template strings quite often, so I need the strings to be easily maintainable without getting headaches every time.

Once the templates are more or less 'done', I'll try to update everything to multiline strings like you suggested.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

No branches or pull requests

2 participants