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鈥檒l occasionally send you account related emails.

Already on GitHub? Sign in to your account

MNT: Deprecate annexstatus #6413

Merged
merged 4 commits into from
Feb 8, 2022
Merged

MNT: Deprecate annexstatus #6413

merged 4 commits into from
Feb 8, 2022

Conversation

adswa
Copy link
Member

@adswa adswa commented Feb 3, 2022

This PR works towards #6076, which calls out the AnnexRepo's annexstatus method is undocumented and mostly unused with the exception of tests.

Its remaining usecases are addressed as follows:

  • A few spots (in save and GitRepo) rely on the existence of this method in ds.repo to assert whether or not we're dealing with an AnnexRepo. These spots were changed to instead check for a uuid property, which should exist in AnnexRepos but not in GitRepos
  • A few tests were using annexstatus. Some were able to change to use get_content_annexinfo. For those that didn't, I moved annexstatus into tests/utils as a new test helper get_annexstatus.

馃彔 Internal

  • AnnexRepo's internal annexstatus is deprecated. In its place, a new test helper assists the few tests that rely on it.

鈥per

This function was formerly used only in tests, with one exception in save
that relied on its precence as an attribute in ds.repo to determine
whether or not ds.repo was an AnnexRepo or GitRepo instance.

Tests that desperately rely on this, or test its function, have been
adjusted to use the test helper instead.

Work towards datalad#6067
鈥xstatus

The latter is a negligably used, to-be deprecated function, while UUID should
be a reliable indicator of AnnexRepo instances as it is a property of
AnnexRepo
@adswa adswa added the semver-patch Increment the patch version when merged label Feb 3, 2022
@mih mih added the team-gitannex git-annex interface (AnnexRepo, protocols, ...) (https://github.com/datalad/datalad/issues/6365) label Feb 7, 2022
@adswa
Copy link
Member Author

adswa commented Feb 7, 2022

Travis failure seems unrelated

======================================================================

ERROR: datalad.distribution.tests.test_get.test_source_candidate_subdataset
----------------------------------------------------------------------
Traceback (most recent call last):
  File "/tmp/dl-miniconda-chiq2wk1/lib/python3.9/site-packages/nose/case.py", line 134, in run
    self.runTest(result)
  File "/tmp/dl-miniconda-chiq2wk1/lib/python3.9/site-packages/nose/case.py", line 152, in runTest
    test(result)
AssertionError: Test was too slow (took 30.1101s, threshold was 30.0000s)

and Appveyor failure is a coverage glitch:

python -m coverage xml
C:\Python310\python.exe: No module named coverage
Command exited with code 1

Copy link
Member

@mih mih left a comment

Choose a reason for hiding this comment

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

LGTM, thx! Small indentation issue, fix suggested.

datalad/support/annexrepo.py Outdated Show resolved Hide resolved
Copy link
Member

@mih mih left a comment

Choose a reason for hiding this comment

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

I merged the doc-fix. Once tests passed, this can move in. Thx!

@codeclimate
Copy link

codeclimate bot commented Feb 8, 2022

Code Climate has analyzed commit dfae470 and detected 0 issues on this pull request.

View more on Code Climate.

@@ -309,7 +309,7 @@ def save_ds(args, version_tag=None):
# calls
paths=None,
# prevent whining of GitRepo
git=True if not hasattr(ds.repo, 'annexstatus')
git=True if not hasattr(ds.repo, 'uuid')
Copy link
Member

Choose a reason for hiding this comment

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

I'd prefer to use that occasion to unify this kind of test:

Suggested change
git=True if not hasattr(ds.repo, 'uuid')
git=True if not isinstance(ds.repo, AnnexRepo)

Copy link
Member

Choose a reason for hiding this comment

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

Not a blocker, though.

Copy link
Member

Choose a reason for hiding this comment

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

Let's do this elsewhere.

@codecov
Copy link

codecov bot commented Feb 8, 2022

Codecov Report

Merging #6413 (dfae470) into master (5452903) will increase coverage by 0.05%.
The diff coverage is 100.00%.

Impacted file tree graph

@@            Coverage Diff             @@
##           master    #6413      +/-   ##
==========================================
+ Coverage   89.94%   90.00%   +0.05%     
==========================================
  Files         344      345       +1     
  Lines       43291    43312      +21     
==========================================
+ Hits        38939    38983      +44     
+ Misses       4352     4329      -23     
Impacted Files Coverage 螖
datalad/core/local/save.py 98.91% <酶> (酶)
datalad/support/annexrepo.py 91.32% <酶> (-0.26%) 猬囷笍
datalad/core/local/tests/test_save.py 98.02% <100.00%> (酶)
datalad/support/gitrepo.py 90.37% <100.00%> (酶)
datalad/support/tests/test_fileinfo.py 100.00% <100.00%> (酶)
datalad/support/tests/test_repo_save.py 100.00% <100.00%> (酶)
datalad/tests/utils.py 89.44% <100.00%> (+0.04%) 猬嗭笍
datalad/core/local/run.py 97.80% <0.00%> (酶)
datalad/support/json_py.py 98.86% <0.00%> (酶)
... and 9 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 5452903...dfae470. Read the comment docs.

@mih mih merged commit b5395dc into datalad:master Feb 8, 2022
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 team-gitannex git-annex interface (AnnexRepo, protocols, ...) (https://github.com/datalad/datalad/issues/6365)
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

3 participants