General cleanup #159
General cleanup #159
Conversation
I have no idea what that does, so I’m just gonna cry and make another coffee now <_<
| except: | ||
| print "NOP" | ||
| print "NOPE" |
There was a problem hiding this comment.
Does this need to be a print? If not, just make it a pass.
There was a problem hiding this comment.
@AWegnerGitHub I don't understand why this is a thing here.... /me shrugs
There was a problem hiding this comment.
The whole point of this was debugging in the beginning to note that nothing occurred here. (see https://en.wikipedia.org/wiki/NOP) Likely, we can just remove this, and make it a pass as @AWegnerGitHub suggests.
Please separate that out into another PR - that way, it can be merged easily & there's a clear history of why (adding it as part of a cleanup commit is kinda strange). |
| if is_spam: | ||
| try: | ||
| handle_spam(title, body, owner_name, site, link, owner_link, a_id, reason, True, why, owner_rep, post_score, up_vote_count, down_vote_count, q_id) | ||
| handle_spam( | ||
| title, |
There was a problem hiding this comment.
You've got weird indentation here - arguments should be indented from the method name if you're using arguments on new lines; otherwise (see hichris' comment) they should just be on the same line as the method call.
|
I've added a number of line notes, most of which are purely stylistic things. This is a lot of work, so thanks for doing it :) |
| message_url = "//chat." + wrap2.host + "/transcript/message/" + str(message_id) | ||
| message_url = "//chat.{host}/transcript/message/{id}#{id}".format( | ||
| host=wrap2.host, | ||
| id=str(message_id) |
There was a problem hiding this comment.
string.format should be able to do automatic casting of integers to strings, is there any reason why we aren't using that here (and other places)?
The-Quill
changed the title
| reasons_copy_copy.remove(reason) | ||
| return len(reasons_copy_copy) == 0 | ||
| reasons_comparison = [r for r in list(reasons_copy) if "username" not in r] | ||
| return len(reasons_comparison) == 0 |
There was a problem hiding this comment.
Wouldn't it make sense to use any here?
return any(r for r in set(reasons) if "username" not in r)Also, you shouldn't need to call list if you are making it a set ahead of time.
There was a problem hiding this comment.
That's code from before me <_< I'll take a look though
|
|
||
| return "Recorded answer as an NAA in metasmoke." | ||
|
|
||
| if ( |
There was a problem hiding this comment.
Leave this all on one line -- it looks pretty ugly this way
There was a problem hiding this comment.
Yeah... flake8 was really digging into me there if I changed it from a oneliner <_<
|
In general this seems to look much better, I'd like to take another look in the morning though. (Probably the others would like to weigh in too) Also, note to self/whoever pulls this in: make sure to squash the commits. |
| if second_part_lower.startswith("ignore") and is_privileged(ev_room, ev_user_id, wrap2): | ||
| if post_site_id is None: | ||
| return "That message is not a report." | ||
|
|
||
| t_metasmoke = Thread(target=Metasmoke.send_feedback_for_post, | ||
| args=(post_url, second_part_lower, ev_user_name, ev_user_id, )) | ||
| t_metasmoke = Thread(target=Metasmoke.send_feedback_for_post, args=(post_url, second_part_lower, ev_user_name, ev_user_id,)) |
There was a problem hiding this comment.
Keep this as a two liner, looks prettier to me. :)
| return None | ||
| else: | ||
| return "That configuration doesn't exist." | ||
| return "I will no longer ping you if I report a post on `{site_name}`, in room `{room_id}` on `chat.{chat_domain}`".format( |
There was a problem hiding this comment.
Can you put this all on one line?
I did some general variable name cleanup, syntax cleanup etc
The code was tested with the test modules and by running in chat....