Skip to content

Microtask 4 #4

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

Open
wants to merge 3 commits into
base: master
Choose a base branch
from
Open

Microtask 4 #4

wants to merge 3 commits into from

Conversation

yashasingh
Copy link
Owner

@yashasingh yashasingh commented Oct 25, 2017

@tgr
Sir,
This tool provides edit quality and article quality analysis of last 5 edits made by user in english Wiki (using ORES API).
Tool- link

Please review.
Thank you.

App/views.py Outdated
'list':'usercontribs',
'uclimit':'5',
'ucuser':username}
url_parameters = urllib.parse.urlencode(parameters)
Copy link

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

You should just use requests.get('https://en.wikipedia.org/w/api.php', params=parameters) and requests does the encoding for you.

Copy link
Owner Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Okay sir.

App/views.py Outdated
response = requests.get(url1)

if response.status_code == 200:
lst = json.loads(response.text)
Copy link

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

You'd probably want a try...catch around this as well, in case the API returns invalid JSON.

Copy link
Owner Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

That was my fault. Rectified.

App/views.py Outdated
{"error": "0 edits found for username {}".format(username)})

for articles in contributions:
url2 = "https://ores.wikimedia.org/v3/scores/enwiki/" + str(articles['revid'])
Copy link

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

YMMV but I find string interpolation more readable than concatenation - you can see the string structure at a glance when skimming the code.

Copy link
Owner Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Changed to string formatting. 👍

App/views.py Outdated
)
except:
# Edit data extracted
edits_data = json.loads(response.text)
Copy link

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

You already parsed this a few lines above.

Copy link
Owner Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Rectified.

App/views.py Outdated

if response.status_code == 200:
lst = json.loads(response.text)
try:
Copy link

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This is a weird way to use exceptions. Normally you would put the normal behavior in the try branch and the error handling in the except branch. In part because it's easier for others to understand your thinking that way, in part because the main branch tends to be longer and involve multiple types of errors. So now you have an except branch which itself can throw exceptions... not a good thing.

Also, specifically for dictionary existence checks, you can just use 'error' in lst or lst.get('error'), neither of which can throw exceptions.

Copy link
Owner Author

@yashasingh yashasingh Oct 29, 2017

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Yes sir, it should have been that way. I have corrected it now.

@yashasingh
Copy link
Owner Author

@tgr
Sir, I have updated the code.
Please review.
Sorry for the mistakes.

Copy link

@tgr tgr left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Looks good!

App/views.py Outdated
# Edit data extracted
edits_data = json.loads(response.text)
if 'error' in edits_data:
return(render(request,
Copy link

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This works just fine, but you could also just do something like

if 'error' in edits_data:
    raise Error(edits_data['error']['info'])

which is arguably easier to read (all the error rendering happens in one place). As your code becomes more complex, you might want to separate rendering form the main control flow (use mode-view-controller or something similar) and exceptions are one way to handle it.

Copy link
Owner Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Thanks for the suggestion. I have corrected code to handle all error rendering in except branch.

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

Successfully merging this pull request may close these issues.

2 participants