Skip to content
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

MNT: github: Add compatibility wrapper for GithubException #5603

Merged
merged 1 commit into from Apr 26, 2021

Conversation

kyleam
Copy link
Contributor

@kyleam kyleam commented Apr 26, 2021

A few spots in the code base instantiate GithubException with two
arguments, but in PyGithub v1.55 (released today) GithubException and
its subclasses require an additional argument (headers).

Add a helper that tries to create an exception with three arguments,
falling back to two arguments for compatibility. Note that using None
for the headers follows what was done in the upstream commit,
ddd437a7c (Export headers in GithubException).

Closes #5602.

A few spots in the code base instantiate GithubException with two
arguments, but in PyGithub v1.55 (released today) GithubException and
its subclasses require an additional argument (`headers`).

Add a helper that tries to create an exception with three arguments,
falling back to two arguments for compatibility.  Note that using None
for the headers follows what was done in the upstream commit,
ddd437a7c (Export headers in GithubException).

Closes datalad#5602.
@codecov
Copy link

codecov bot commented Apr 26, 2021

Codecov Report

Merging #5603 (986b674) into maint (d55866d) will decrease coverage by 39.86%.
The diff coverage is 66.66%.

❗ Current head 986b674 differs from pull request most recent head ae7648b. Consider uploading reports for the commit ae7648b to get more accurate results
Impacted file tree graph

@@             Coverage Diff             @@
##            maint    #5603       +/-   ##
===========================================
- Coverage   90.24%   50.38%   -39.87%     
===========================================
  Files         299      296        -3     
  Lines       42224    42194       -30     
===========================================
- Hits        38106    21258    -16848     
- Misses       4118    20936    +16818     
Impacted Files Coverage Δ
datalad/support/github_.py 37.34% <57.14%> (-34.56%) ⬇️
datalad/support/tests/test_github_.py 86.76% <100.00%> (ø)
datalad/support/tests/utils.py 0.00% <0.00%> (-100.00%) ⬇️
datalad/plugin/tests/__init__.py 0.00% <0.00%> (-100.00%) ⬇️
datalad/cmdline/tests/__init__.py 0.00% <0.00%> (-100.00%) ⬇️
datalad/interface/tests/__init__.py 0.00% <0.00%> (-100.00%) ⬇️
datalad/interface/tests/test_diff.py 0.00% <0.00%> (-100.00%) ⬇️
datalad/interface/tests/test_docs.py 0.00% <0.00%> (-100.00%) ⬇️
datalad/cmdline/tests/test_helpers.py 0.00% <0.00%> (-100.00%) ⬇️
datalad/interface/tests/test_clean.py 0.00% <0.00%> (-100.00%) ⬇️
... and 201 more

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update d55866d...ae7648b. Read the comment docs.

Copy link
Member

@yarikoptic yarikoptic left a comment

Choose a reason for hiding this comment

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

I would have better checked for external_versions['github'] <= '1.5' but apparently github doesn't provide __version__ and our current logic there which falls fallback to pkg_resources

$> python -c 'from datalad.support.external_versions import external_versions as ev; print(ev["github"])'
UNKNOWN

is not good enough since the distribution package is named pygithub and not github.

So I guess for now let's just stick with duck typing but I will submit a PR to resolve this versioning gotcha

@kyleam kyleam merged commit 9e1af30 into datalad:maint Apr 26, 2021
@kyleam kyleam deleted the pygithub-incompat branch April 26, 2021 20:23
@yarikoptic yarikoptic added the semver-patch Increment the patch version when merged label Jun 2, 2021
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
semver-patch Increment the patch version when merged
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

2 participants