Skip to content

Conversation

@reiven
Copy link
Contributor

@reiven reiven commented Aug 1, 2017

No description provided.

@coveralls
Copy link

coveralls commented Aug 1, 2017

Coverage Status

Coverage remained the same at 100.0% when pulling b57c2cf on reiven:master into 8261502 on vimalloc:master.

Copy link
Owner

@vimalloc vimalloc left a comment

Choose a reason for hiding this comment

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

Added a couple of minor tweaks, if you don't mind addressing them. Thanks for contributing! :)


if request.is_json:
params = request.get_json()
if 'username' in params.keys() and 'password' in params.keys():
Copy link
Owner

@vimalloc vimalloc Aug 1, 2017

Choose a reason for hiding this comment

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

Can you change this to not use .keys()? Doing 'username' in params is more efficient (at least with python2), as .keys() makes a copy of the keys into a list, which you then have to iterate through (O(n) instead of O(1))

Copy link
Contributor Author

Choose a reason for hiding this comment

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

sure

ret = {'access_token': create_access_token(identity=params['username'])}
return jsonify(ret), 200
else:
return jsonify({"msg": "Missing auth"}), 401
Copy link
Owner

Choose a reason for hiding this comment

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

Just to avoid a bunch of nested if's, can you lift this to the top? Something like

if not request.is_json:
    return jsonify({"msg": "Missing auth"}), 401

# rest of the code, but not nested in an if block any more

@coveralls
Copy link

coveralls commented Aug 2, 2017

Coverage Status

Coverage remained the same at 100.0% when pulling f42956d on reiven:master into 8261502 on vimalloc:master.

@vimalloc vimalloc merged commit 3596b6b into vimalloc:master Aug 3, 2017
@vimalloc
Copy link
Owner

vimalloc commented Aug 3, 2017

Thanks for contributing 👍

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.

3 participants