8000 Removed tests/test_repos.py by itsmemattchung · Pull Request #565 · sigmavirus24/github3.py · GitHub
[go: up one dir, main page]

Skip to content

Removed tests/test_repos.py #565

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

Merged
merged 3 commits into from
Nov 24, 2016

Conversation

itsmemattchung
Copy link
Contributor

Removed duplicated TestAsset in tests_repos_repos; TestAsset exists in
test_repos_release. Added test_str, and test_download to TestAsset.

Updated repos_asset_example's JSON.

@@ -227,7 +227,7 @@ def download(self, path=''):
}
resp = self._get(self._api, allow_redirects=False, stream=True,
headers=headers)
if resp.status_code == 302:
if resp and resp.status_code == 302:
Copy link
Owner

Choose a reason for hiding this comment

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

So _get and the other methods that proxy to requests always return a Response (unless requests itself raises an exception). We shouldn't need to check for None here since the only time we won't have a Response is in the exceptional case in which case we also won't be exercising this code. :)

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Good point

@sigmavirus24
Copy link
Owner

Just a couple comments.

Removed duplicated TestAsset in tests_repos_repos; TestAsset exists in
test_repos_release.  Added test_str, and test_download to TestAsset.

Updated repos_asset_example's JSON.
Per @sigmavirus24, proxy to requests always return a response unless an exception is raised.
@itsmemattchung
Copy link
Contributor Author

@sigmavirus24 I made one modification and responded to the suggestions; let me know if you still want me to make those changes :)

@sigmavirus24 sigmavirus24 merged commit e233097 into sigmavirus24:develop Nov 24, 2016
@itsmemattchung
Copy link
Contributor< 75A1 /span> Author

🌵

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
0