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

RF: AnnexRepo.get_corresponding_branch() -> None without a managed branch #4274

Merged
merged 1 commit into from Mar 10, 2020

Conversation

mih
Copy link
Member

@mih mih commented Mar 10, 2020

When dealing with managed branches, one frequently has to switch
behavior depending on their presence. With this change the return value
of get_corresponding_branch() carries the information whether there is
such a branch or not, and immediately allows decision making based on
it.

Previously, it required an additional call to is_managed_branch()
(that is duplicated in get_corresponding_branch() in order to determine
this condition.

This cost reduction is minimal, but the code complexity goes down.
The previous behavior is easily achieved via

`get_corresponding_branch(branch) or branch`

and seem to communicate more direct what is intended.

A GitRepo.get_corresponding_branch() that always returns None has been
added to further simplify decision making and avoid type-based
switching.

…anch

When dealing with managed branches, one frequently has to switch
behavior depending on their presence. With this change the return value
of get_corresponding_branch() carries the information whether there is
such a branch or not, and immediately allows decision making based on
it.

Previously, it required an additional call to is_managed_branch()
(that is duplicated in get_corresponding_branch() in order to determine
this condition.

This cost reduction is minimal, but the code complexity goes down.
The previous behavior is easily achieved via

    `get_corresponding_branch(branch) or branch`

and seem to communicate more direct what is intended.

A GitRepo.get_corresponding_branch() that always returns None has been
added to further simplify decision making and avoid type-based
switching.
@codecov
Copy link

codecov bot commented Mar 10, 2020

Codecov Report

Merging #4274 into master will decrease coverage by 0.01%.
The diff coverage is 87.50%.

Impacted file tree graph

@@            Coverage Diff             @@
##           master    #4274      +/-   ##
==========================================
- Coverage   88.71%   88.70%   -0.02%     
==========================================
  Files         282      282              
  Lines       36928    36930       +2     
==========================================
- Hits        32759    32757       -2     
- Misses       4169     4173       +4     
Impacted Files Coverage Δ
datalad/support/gitrepo.py 90.62% <50.00%> (-0.07%) ⬇️
datalad/distribution/tests/test_update.py 97.72% <100.00%> (ø)
datalad/support/annexrepo.py 85.29% <100.00%> (-0.25%) ⬇️
datalad/support/tests/test_annexrepo.py 95.27% <100.00%> (ø)

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 4e89de4...8473249. Read the comment docs.

kyleam
kyleam approved these changes Mar 10, 2020
Copy link
Collaborator

@kyleam kyleam left a comment

Thanks for proposing this. I prefer the None return behavior as well, and in my view this function is obscure enough that we can make this change without worrying too much about compatibility.

I'm happy to see the GitRepo.get_corresponding_branch addition.

@@ -756,8 +756,7 @@ def check_merge_follow_parentds_subdataset_detached(on_adjusted, path):
# the explicit revision fetch fails.
(ds_src_s1.pathobj / "bar").write_text("bar content")
ds_src.save(recursive=True)
ds_src_s1.repo.checkout(
ds_src_s1.repo.get_corresponding_branch("master"))
ds_src_s1.repo.checkout('master')
Copy link
Collaborator

@kyleam kyleam Mar 10, 2020

Choose a reason for hiding this comment

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

Eh, that was silly of me. I guess in some earlier iteration that was a plain .get_corresponding_branch(), and I didn't drop the call when I switched to explicitly using "master", or something :x

@mih
Copy link
Member Author

mih commented Mar 10, 2020

Thanks @kyleam

@mih mih merged commit f0d05e3 into datalad:master Mar 10, 2020
10 of 18 checks passed
@mih mih deleted the rf-corresponding branch Mar 10, 2020
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