-
Notifications
You must be signed in to change notification settings - Fork 981
Populate null columns in repo_info table #3584
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
base: main
Are you sure you want to change the base?
Populate null columns in repo_info table #3584
Conversation
Signed-off-by: guptapratykshh <[email protected]>
sgoggins
left a comment
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.
I would like to see Unit tests written for these functions in a way that validates the existence of the end point and incorporates the use of edge cases. i.e. valid cases with an empty URL, invalid cases where there is not a 404 but its empty, etc.).
| 'issues_enabled': data['hasIssuesEnabled'] if 'hasIssuesEnabled' in data else None, | ||
| 'open_issues': data['issues']['totalCount'] if data['issues'] else None, | ||
| 'pull_requests_enabled': None, | ||
| 'pull_requests_enabled': 'true' if repo_data.get('has_projects') else 'false', |
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.
We are not using the .get syntax elsewhere. I am not sure I love the nesting, and I agree it is useful to get these data elements we previously ignored.
Can you share a rationale for this design choice?
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.
get syntax is better because it wont crash if the key doesnt exist (will return a default value, which is none by default)
I use it all the time
Unit tests for API dependent stuff is... a little trickier to do. especially if the function being tested is making the web calls on its own, not expecting the data to be passed into them. |
|
This will also conflict with #3597. Im going to say that this should wait for that PR to merge, and then be adjusted to reuse the same function for fetching the correct domain name to use in Github API calls. |
|
Hello! Just wanted to check in to see if you were still interested in helping the maintainers merge this PR. We noticed it has been a little while since this last had activity, and are considering closing it or taking it over if it remains in its current state. Please react to or reply to this to confirm your interest in the next 7 days or let us know if you are no longer interested in this so we can best prioritize everyone's contributions. Thanks! |
Description
added data collection for six null columns in repo_info table -
contributing_file,security_issue_file,changelog_file,keywords,pull_requests_enabled, andpages_enabled.This PR fixes #2675
Notes for Reviewers
Signed commits