-
-
Notifications
You must be signed in to change notification settings - Fork 771
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
General faucet cleanup #582
Conversation
Codecov Report
@@ Coverage Diff @@
## master #582 +/- ##
=========================================
Coverage ? 29.15%
=========================================
Files ? 93
Lines ? 4856
Branches ? 568
=========================================
Hits ? 1416
Misses ? 3393
Partials ? 47
Continue to review full report at Codecov.
|
@owocki When you have a moment, can I get 👀 on this? |
'user': user['items'][0] | ||
} | ||
response = {'status': 200, 'user': False} | ||
if not len(user['items']) == 0 or not user['items'][0]['login'].lower() != profile.lower(): |
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.
this looks different than what was being done before?
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.
It is. I'm checking if not for both cases, defaulting to {'status': 200, 'user': False}
and only updating response['user']
if the case is correct ^ - It's performing the same logic, just updating the one value if necessary to be more DRY.
return JsonResponse({ | ||
'message': e.messages[0] | ||
}, status=400) | ||
github_profile = request.POST.get('githubProfile') |
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.
did not test here either.. can you do a runthrough of testing?
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.
It's doing the same exact thing, only catching in one place versus three since we're returning on exception anyway.
just left a few comments.. LGTM once its tested both success/fail cases |
@owocki The logic hasn't really changed. As for the python code, it's the same exact thing as before only a little more DRY. |
🚢 ed-- thanks! |
Description
The goal of this PR is to cleanup some of the logic and syntax associated with
Faucet
and add docstrings.Checklist
Affected core subsystem(s)
faucet