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

BF: introduce GitTransportRI - a basic implementation only #4529

Merged
merged 2 commits into from May 14, 2020

Conversation

yarikoptic
Copy link
Member

this should be sufficient to address #4489
to avoid treating such RIs as ssh addresses. Current implementation of
GitTransportRI simply splits apart transport and underlying RI attributes.
Since directed toward maint I have refrained from doing any code refactoring, such as
in RI().init and in the tests check() to avoid using "ri" (and other) arguments
which might collide with fields provided in **kwargs. May be in some next sweep
those all could become prefixed (or suffixed) with _ to avoid such collisions.

We could also make this GitTransportRI immediately turn underlying RI into an
instance (ATM it is just a string), and also expose .path as all other RIs
apparently do. But again, for the sake of the quick fix for now to not cause
hard to decypher errors, I decided to postpone that.

Note: I have not attempted to actually use it in some sample case fully since
e.g. our create-sibling-github has no clue about transport::, would need to
override url it provides for the remote and may be more...

Closes #4489

this should be sufficient to address datalad#4489
to avoid treating such RIs as ssh addresses.  Current implementation of
GitTransportRI simply splits apart transport and underlying RI attributes.
Since directed toward maint I have refrained from doing any code refactoring, such as
in RI().__init__ and in the tests check() to avoid using "ri" (and other) arguments
which might collide with fields provided in **kwargs.  May be in some next sweep
those all could become prefixed (or suffixed) with  _ to avoid such collisions.

We could also make this GitTransportRI immediately turn underlying RI into an
instance (ATM it is just a string), and also expose .path as all other RIs
apparently do.  But again, for the sake of the quick fix for now to not cause
hard to decypher errors, I decided to postpone that.

Note: I have not attempted to actually use it in some sample case fully since
e.g. our create-sibling-github has no clue about transport::, would need to
override url it provides for the remote and may be more...
@yarikoptic yarikoptic added this to the 0.12.7 milestone May 14, 2020
@codecov
Copy link

codecov bot commented May 14, 2020

Codecov Report

Merging #4529 into maint will increase coverage by 47.27%.
The diff coverage is 100.00%.

Impacted file tree graph

@@             Coverage Diff             @@
##            maint    #4529       +/-   ##
===========================================
+ Coverage   42.41%   89.68%   +47.27%     
===========================================
  Files         275      275               
  Lines       37105    37122       +17     
===========================================
+ Hits        15738    33294    +17556     
+ Misses      21367     3828    -17539     
Impacted Files Coverage Δ
datalad/support/network.py 86.90% <100.00%> (+6.67%) ⬆️
datalad/support/tests/test_network.py 100.00% <100.00%> (+1.83%) ⬆️
datalad/tests/test_archives.py 95.00% <0.00%> (+0.07%) ⬆️
datalad/core/local/create.py 94.77% <0.00%> (+0.74%) ⬆️
datalad/core/local/tests/test_create.py 100.00% <0.00%> (+0.90%) ⬆️
datalad/ui/progressbars.py 83.10% <0.00%> (+1.35%) ⬆️
datalad/support/external_versions.py 95.62% <0.00%> (+1.45%) ⬆️
datalad/dochelpers.py 87.40% <0.00%> (+1.48%) ⬆️
datalad/core/local/status.py 98.03% <0.00%> (+1.96%) ⬆️
... and 188 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 b80d930...1ba19cb. Read the comment docs.

Copy link
Contributor

@kyleam kyleam left a comment

Choose a reason for hiding this comment

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

Thanks for nice analysis and the fix. This looks good to me.

datalad/support/network.py Outdated Show resolved Hide resolved
@kyleam kyleam merged commit 3208a38 into datalad:maint May 14, 2020
@yarikoptic yarikoptic deleted the bf-git-transport-ri branch May 21, 2020 17:41
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.

None yet

2 participants