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

Non-GitPy GitRepo.get_branch_commits() #3834

Merged
merged 1 commit into from Feb 20, 2020
Merged

Conversation

mih
Copy link
Member

@mih mih commented Oct 24, 2019

This is sitting on top of gh-3833. And aims to be another step towards #2970 by fixing #3813

Sibling PR in the crawler is datalad/datalad-crawler#66

@codecov
Copy link

@codecov codecov bot commented Oct 29, 2019

Codecov Report

Merging #3834 into master will increase coverage by 0.04%.
The diff coverage is n/a.

Impacted file tree graph

@@            Coverage Diff             @@
##           master    #3834      +/-   ##
==========================================
+ Coverage   89.13%   89.17%   +0.04%     
==========================================
  Files         275      275              
  Lines       35847    35825      -22     
==========================================
- Hits        31953    31948       -5     
+ Misses       3894     3877      -17     
Impacted Files Coverage Δ
datalad/support/tests/test_annexrepo.py 95.57% <0.00%> (+0.32%) ⬆️
datalad/downloaders/base.py 75.55% <0.00%> (+0.37%) ⬆️
datalad/downloaders/http.py 74.10% <0.00%> (+0.39%) ⬆️
datalad/downloaders/tests/test_http.py 61.80% <0.00%> (+1.21%) ⬆️
datalad/support/tests/utils.py 96.15% <0.00%> (+11.53%) ⬆️
datalad/support/tests/test_cookies.py 100.00% <0.00%> (+14.28%) ⬆️

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 bc05be3...c49782b. Read the comment docs.

@mih mih marked this pull request as ready for review Oct 29, 2019
@bpoldrack
Copy link
Member

@bpoldrack bpoldrack commented Oct 29, 2019

LGTM

Copy link
Contributor

@kyleam kyleam left a comment

The changes themselves look fine to me, but the crawler uses get_branch_commits(). So we either need to (1) prepare an adjustment PR against the crawler or (2) keep get_branch_commits() around for now (or both). The crawler already contains several unreleased compatibility fixes, so I think no. 1 would be fine.

mih added a commit to datalad/datalad-crawler that referenced this issue Nov 29, 2019
Repo classes have no `get_branch_commits()` anymore. The crawler
uses the test-helper replacement now. it could eventually also
swallow that function into its code base.
@mih
Copy link
Member Author

@mih mih commented Nov 29, 2019

I prepared an adjustment PR for the crawler datalad/datalad-crawler#66

I would merge this when the test have gone through.

@@ -1827,6 +1827,37 @@ def newfunc(*args, **kwargs):
return newfunc


def get_branch_commits(repo, branch=None, limit=None, stop=None):
"""Return commit hexshas for a branch
Copy link
Member

@yarikoptic yarikoptic Dec 3, 2019

Choose a reason for hiding this comment

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

My main question is - why not to keep it as a method (well, rename to get_branch_commits_) of GitRepo and just remove value parameter, thus always yielding hexshas?
or we do not foresee any need for such functionality for anything but the tests? I already see one usecase RFed above in https://github.com/datalad/datalad/pull/3834/files#diff-18445826672fc8f047d0b7d56512986cR327 for direct call to git (IMHO suboptimal to breed those)

Copy link
Member Author

@mih mih Dec 3, 2019

Choose a reason for hiding this comment

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

The idea here, and in other PRs before, is to get rid all these tiny wrapper around simple git calls and leave them to the return value focused wrappers we have now. I was under the impression that this direction had received general support. I dont see why this particular method should be treated differently.

Copy link
Member Author

@mih mih Feb 20, 2020

Choose a reason for hiding this comment

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

I have now implemented what you suggested @yarikoptic and adjusted the companion PR in the crawler too.

@mih mih mentioned this pull request Feb 14, 2020
15 tasks
@mih mih force-pushed the rf-branchcommits branch 2 times, most recently from b66e98f to b9930f1 Compare Feb 20, 2020
mih added a commit to datalad/datalad-crawler that referenced this issue Feb 20, 2020
See datalad/datalad#3834

Compatibility kludges have been put in place, given that the impact on
the crawler is just a rename.
@mih mih changed the title Get rid of GitRepo.get_branch_commits() as a non-test-helper Non-GitPy GitRepo.get_branch_commits() Feb 20, 2020
and add '_' function name suffix to indicate its generator-behavior.

This furthers datalad#2970
@mih
Copy link
Member Author

@mih mih commented Feb 20, 2020

I have dropped all changes that removed this method from the class and turned into a standalone function.
Remaining changes are:

  • rename to match generator-method convention
  • stripping the flexibility of the method down to what is needed in core and crawler, and reimplement it with rev-list
  • adjust usage due to those changes

@mih
Copy link
Member Author

@mih mih commented Feb 20, 2020

As with other PRs the windows github ci failure is unrelated. Crawler failure is addressed in companion PR.

@mih mih merged commit 8f4f7c2 into datalad:master Feb 20, 2020
15 of 17 checks passed
@mih mih deleted the rf-branchcommits branch Feb 20, 2020
mih added a commit to datalad/datalad-crawler that referenced this issue Feb 25, 2020
See datalad/datalad#3834

Compatibility kludges have been put in place, given that the impact on
the crawler is primarily a rename.
mih added a commit to datalad/datalad-crawler that referenced this issue Feb 25, 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

4 participants