-
Notifications
You must be signed in to change notification settings - Fork 181
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
Refactored report related functionalities #3885
base: master
Are you sure you want to change the base?
Conversation
Refactored report related functionalities to reduce redundancy in code. Also implemented: issue Charcoal-SE#3712 and issue Charcoal-SE#2795 .
6cac5fb
to
c8e4fa4
Compare
I need to look at this more closely, but wanted to pose a general question: The prior organization was that there was a function The The So far, this appears to be heading in the direction of still requiring |
(@makyen ) because allspam does not get a list of urls. It first get a list of accounts a user has, and for each account, it get a list of |
Yes, and no. It still makes a single request for each answer, which are a significant percentage of the posts reported. Admittedly, it could reduce that by making one request for all the answers by the user. But, frankly, the reduction in duplicated code seems worth what is small number of redundant API requests. |
Hmmm... It may be that I'm reacting to the fact that we're manually constructing the post record in at least three different files, without any actual coordination between them and the only documentation as to what it is to contain held in apigetpost.py or by looking at the filters for the SE API calls. If you want to retain getting most/all of the post data from the call to the |
Redundant code removed. Hopefully we won't have more than 10 allspam commands a day, and for those there won't be more than 10 posts each. Hence the trade off in API requests is quite reasonable. |
Well, if we're really concerned about optimizing API requests here, then we could implement something like Overall, there have only been on the order of 550 Thanks for taking care of it. I'm sorry if my comments came off a bit negative. I allowed my frustration with things which are not your responsibility to leak through, which I shouldn't have permitted. |
Stuff to add to refdoc.md:
(to be continued) |
(continued) |
Note: it is better to review the overall file changes and to use the split view.
|
chatcommands.py
Outdated
@@ -1752,6 +1752,16 @@ def allspam(msg, url, alias_used="allspam"): | |||
|
|||
def report_post(url, reported_by, reported_in=None, blacklist_by=None, operation="report", custom_reason=None): | |||
""" Scan or report a single post """ | |||
# Arguments: | |||
# - url: url to the post being reported | |||
# - reported_by: user reporting the post |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
What is reported_by
? A string? A dict? Similar information should be provided for the other arguments too. This should provide enough information such that someone could understand what they need to pass to the function, without them having to look through the function to figure out how it's used.
|
At least from my point of view, the only thing needed to get this PR merged is to have a similar level of detail (or more) in the docstring for |
Please ignore CircleCI failure. The only change is in comments and I can be almost certain that this is a network issue on CircleCI server. |
# if the post wasn't previously reported for the cases where we want to process it | ||
# as such. | ||
# Expand real scan results from dirty returm value when not "!!/scan" | ||
# Presence of "scan_why" indicates the post IS spam but ignored |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Please return these comments to being in the code.
The code performs a unintuitive manipulation of the data here, because what's provided by check_if_spam()
's return value has a couple of different formats, which are really not very clear. I found it frustrating to have to go figure out what the various different posibilities were which it could return in order to have the code do what it's supposed to here. I added the above comment so that someone else doesn't need to repeat that same work.
First, sorry for the delay in getting back to this. I finally did more testing (starting here). As you can see for the In addition, it was a helpful feature that |
Instead of the non-indicative "[Post](<url>): Error message" reply, use the explicit "Post <url>: Error message".
This issue has been closed because it has had no recent activity. If this is still important, please add another comment and find someone with write permissions to reopen the issue. Thank you for your contributions. |
Refactored report related functionalities to reduce redundancy in code. Also implemented: issue #3712 and issue #2795 .