Populate contributor profile fields from GitHub user API (#3694)#3695
Populate contributor profile fields from GitHub user API (#3694)#3695MoralCode merged 3 commits intochaoss:mainfrom
Conversation
sgoggins
left a comment
sgoggins
left a comment
There was a problem hiding this comment.
Do you have specific logs without this fix to illustrate it fixes the bug? Have you tested this? Really appreciate you diving in on this one, it's just more efficient to ask this question than to run it myself. 🫠
| #session.logger.info(f"Contributor: {cntrb} \n") | ||
| batch_insert_contributors(logger, [cntrb]) | ||
|
|
||
| except Exception as e: |
There was a problem hiding this comment.
@MoralCode : Is this how we are propagating errors now? I'm asking because I know it's how we did that originally, and I know @ABrain7710 had at one point advised a different strategy. Time for a style guide.
There was a problem hiding this comment.
I kept the existing pattern in this file (log then raise e) and didn’t change the error handling. Happy to follow whatever strategy you and @ABrain7710 prefer for propagating errors and to update this PR if you decide on a style.
There was a problem hiding this comment.
I don't think we've edited this file in a spell, so I'm more questioning if there's a new style we are using that we should implement while we are here. So, that's not at all a critique of what you did. Following what's already in file is exactly what I would have done. I'm more asking other maintainers so we take the opportunity of this pr to pay down tech debt. This might be how we are doing it now ... I've been on augur for too long to be sure without asking Andrew and Adrian.
There was a problem hiding this comment.
I don't have a style guide currently available that i know about. I think that would make a good compliment to #3662. I don't think we should enforce style guidelines that are undocumented as that isn't fair to new contributors.
- Fix NameError when email is missing: set canonical_email from email (both can be None) so contributor insert never fails on missing email. - Use .get() for all optional profile fields (email, name, company, location, created_at, updated_at, and all gh_* URL fields) so every available value from the user endpoint is stored in the contributors table. Signed-off-by: antcybersec <anant1234466@gmail.com>
…#3694) Signed-off-by: antcybersec <anant1234466@gmail.com>
antcybersec
force-pushed
the
fix-contributor-profile-fields
branch
from
eb1f7cf to
0cde835
Compare
|
@antcybersec are you on slack? |
MoralCode
left a comment
MoralCode
left a comment
There was a problem hiding this comment.
This looks good to me. Remove the two new script files and ill change this to approved.
MoralCode
added
the
pending changes
Signed-off-by: antcybersec <anant1234466@gmail.com>
MoralCode
left a comment
MoralCode
left a comment
There was a problem hiding this comment.
approve. Havent personally tested but the code looks fine at least
MoralCode
added
testing
|
I checked out the PR branch locally and tested ingestion on psf/requests. 
all runnning good now let me verify before the test log if error happening or not |
shlokgilda
left a comment
shlokgilda
left a comment
There was a problem hiding this comment.
Solid fix. Left a minor inline comment, nothing blocking. Looks good to merge once that's considered.
| #"data_source": session.data_source | ||
| "gh_url": contributor.get('url'), | ||
| "gh_html_url": contributor.get('html_url'), | ||
| "gh_node_id": contributor.get('node_id'), |
There was a problem hiding this comment.
The old code had a comment here noting this field is used for duplicate checking. Might be worth keeping that context since it's not obvious from the field name alone.
| "gh_node_id": contributor.get('node_id'), | |
| "gh_node_id": contributor.get('node_id'), # Used for duplicate checking |
There was a problem hiding this comment.
this is documenting a usecase in another part of augur's code, rather than something from github and is minor enough that im inclined to just let it go. if its being used, im sure a ctrl-f for it in the augur code will help locate the code using it.
5 participants
Description
This PR fixes #3694
Notes for Reviewers
Signed commits