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

Core Repo/Dataset classes #4689

Closed
wants to merge 17 commits into from
Closed

Core Repo/Dataset classes #4689

wants to merge 17 commits into from

Conversation

mih
Copy link
Member

@mih mih commented Jul 11, 2020

Merely the first steps towards a resolution of gh-3988 in the spirit of gh-3192, hopefully and eventually leading to a sliming/modernization of the public API (gh-4589).

There is not much happening here, other than moving code around and establishing the classes. The plan is to lower the threshold to transition some carefully filtered functionality in the core API. At the same time the main Repo classes will be used primarily for a long time still. But it should become progressively easier to write code that only uses the core API.

Major pieces:

  • core repo classes GitRepo and AnnexRepo (i.e. identically named as suggested in Establish core *Repo classes #3988 (comment))
  • call_git*() methods have moved and no longer rely on an execution helper that invokes normalize_paths()
  • transition a minimal additional set of functionality to make the flyweight pattern work with the core API only
  • not moving code that directly uses wrapt

Fixes gh-3988

I am currently pondering whether we might also want to established corresponding locations for core utils, exceptions and support with the analog goal of a rejuvenation/slimming of the code base. But later and in a separate effort.

@mih mih changed the title Core Repo classes Core Repo/Dataset classes Jul 11, 2020
@mih
Copy link
Member Author

mih commented Jul 12, 2020

This is now about as far as I'd go for a meaningful first step.

Copy link
Member

@bpoldrack bpoldrack left a comment

Choose a reason for hiding this comment

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

Looks good to me as a first move, except for the os.path issue I commented on. I think at least we shouldn't encourage users of the python API (or ourselves for that matter) to use it over pathobj.

datalad/core/dataset/dataset.py Show resolved Hide resolved
datalad/core/dataset/gitrepo.py Outdated Show resolved Hide resolved
@codecov
Copy link

codecov bot commented Jul 13, 2020

Codecov Report

Merging #4689 into master will decrease coverage by 0.12%.
The diff coverage is 95.77%.

Impacted file tree graph

@@            Coverage Diff             @@
##           master    #4689      +/-   ##
==========================================
- Coverage   89.71%   89.58%   -0.13%     
==========================================
  Files         289      291       +2     
  Lines       40455    40231     -224     
==========================================
- Hits        36295    36042     -253     
- Misses       4160     4189      +29     
Impacted Files Coverage Δ
datalad/support/gitrepo.py 89.55% <89.65%> (-0.95%) ⬇️
datalad/core/dataset/utils.py 90.47% <90.47%> (ø)
datalad/core/dataset/annexrepo.py 90.90% <90.90%> (ø)
datalad/core/dataset/dataset.py 96.84% <96.84%> (ø)
datalad/core/dataset/gitrepo.py 97.14% <97.14%> (ø)
datalad/core/dataset/__init__.py 96.77% <100.00%> (ø)
datalad/core/local/repo.py 100.00% <100.00%> (+9.52%) ⬆️
datalad/customremotes/tests/test_archives.py 89.93% <100.00%> (-0.07%) ⬇️
datalad/distribution/dataset.py 93.29% <100.00%> (-1.13%) ⬇️
...lad/distribution/tests/test_create_test_dataset.py 100.00% <100.00%> (ø)
... and 69 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 3d4d788...7c61a0a. 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.

Glancing through --color-moved diffs, the code movement looks fine to me, though admittedly my eyes glazed over at some point.

datalad/core/dataset/gitrepo.py Outdated Show resolved Hide resolved
datalad/core/dataset/gitrepo.py Outdated Show resolved Hide resolved
datalad/core/dataset/gitrepo.py Outdated Show resolved Hide resolved
datalad/core/dataset/gitrepo.py Outdated Show resolved Hide resolved
datalad/core/dataset/gitrepo.py Outdated Show resolved Hide resolved
datalad/core/dataset/gitrepo.py Outdated Show resolved Hide resolved
datalad/support/gitrepo.py Show resolved Hide resolved
@mih
Copy link
Member Author

mih commented Jul 14, 2020

I pushed a series of commits that address all raised points. I will evaluate these changes in the context of #4698 now.

@bpoldrack
Copy link
Member

Seems you stripped path from GitRepo but not from Dataset. Oversight or is there a reason for it?

@mih
Copy link
Member Author

mih commented Aug 27, 2020

Rebased on master after an eternity has passed.

mih added a commit to mih/datalad that referenced this pull request Aug 28, 2020
This is recovering the first set of changes from the stale dataladgh-4689
mih added a commit to mih/datalad that referenced this pull request Aug 31, 2020
Follow-up to dataladgh-4841 to eventually fulfill dataladgh-4689

Diff looks complicated but it is basically moving the function body into
the `try` block to be able to use `call_git_items_()`
mih added a commit to mih/datalad that referenced this pull request Aug 31, 2020
But use `call_git*()` methods instead.

Follow-up to dataladgh-4841 to eventually fulfill dataladgh-4689
mih added a commit to mih/datalad that referenced this pull request Aug 31, 2020
mih added a commit to mih/datalad that referenced this pull request Aug 31, 2020
All `call_git*()` methods are now running `GitWitlessRunner`. Moreover,
there is no `normalize_paths` decorator involvement for any such
invokations anymore (ping dataladgh-3881, dataladgh-4595).

For now `GitRepo._git_custom_command()` is kept -- at least until
dataladgh-4847 and dataladgh-4848 are merged.

Follow-up to dataladgh-4841 to eventually fulfill dataladgh-4689
@mih
Copy link
Member Author

mih commented Oct 4, 2020

This will need to be attempted at a later point again. Too much has changed.

@mih mih closed this Oct 4, 2020
@mih mih deleted the rf-corerepo branch September 24, 2021 10:20
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Establish core *Repo classes
3 participants