datalad icon indicating copy to clipboard operation
datalad copied to clipboard

RF special remote base classes

Open bpoldrack opened this issue 2 years ago • 8 comments

This is refactoring our SpecialRemote and AnnexCustomRemote classes. The latter only serves as base class for the datalad and datalad-archives special remotes, whereas the former is the more general one. Hence, move functionality that isn't specific to those two. Two minor fixes along the way:

  • Implementation of availability was a bit inconsistent
  • Possible failure at the outermost layer wasn't sending an ERROR message to annex, but only logging.

I don't think this needs a changelog.

bpoldrack avatar May 05 '22 09:05 bpoldrack

Codecov Report

Merging #6682 (f95d188) into master (9c6b62c) will increase coverage by 1.46%. The diff coverage is 88.88%.

:exclamation: Current head f95d188 differs from pull request most recent head 1441891. Consider uploading reports for the commit 1441891 to get more accurate results

@@            Coverage Diff             @@
##           master    #6682      +/-   ##
==========================================
+ Coverage   88.83%   90.29%   +1.46%     
==========================================
  Files         353      353              
  Lines       45677    45750      +73     
==========================================
+ Hits        40575    41310     +735     
+ Misses       5102     4440     -662     
Impacted Files Coverage Δ
datalad/customremotes/archives.py 90.25% <ø> (-0.05%) :arrow_down:
datalad/customremotes/main.py 88.23% <0.00%> (+2.52%) :arrow_up:
datalad/customremotes/__init__.py 91.17% <100.00%> (+2.28%) :arrow_up:
datalad/customremotes/base.py 93.65% <100.00%> (-0.47%) :arrow_down:
datalad/local/addurls.py 86.38% <0.00%> (-8.58%) :arrow_down:
datalad/__init__.py 97.95% <0.00%> (+16.32%) :arrow_up:
datalad/tests/utils.py 57.11% <0.00%> (+35.48%) :arrow_up:
datalad/tests/test_tests_utils.py 92.39% <0.00%> (+92.39%) :arrow_up:

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 9c6b62c...1441891. Read the comment docs.

codecov[bot] avatar May 05 '22 10:05 codecov[bot]

I'm not an expert on special remotes, but overall those changes look little controversial to me. In the interest of progressing with existing PRs so that they do not get forgotten, what is @yarikoptic and @bpoldrack's current verdict about the discussion on whether or not to log in addition to printing errors? I don't feel like I could confidently judge all implications of either solution so I'm secretly hopeful that the weeks in between now and that discussion led to a consensus ;-)

adswa avatar May 31 '22 06:05 adswa

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

View more on Code Climate.

codeclimate[bot] avatar Jul 06 '22 09:07 codeclimate[bot]

it was approved, comments addressed... appveyor though so red, can't tell if anything relates to these PR changes. Clicked "rerun incomplete". Travis for some reason is not present at all!

yarikoptic avatar Jul 06 '22 21:07 yarikoptic

Despite approvals, not merging yet. There are failures where tests start querying interactively, which doesn't happen on master apparently. That would suggest I somehow messed with the detection. Postponing until after 0.17.0

bpoldrack avatar Jul 07 '22 12:07 bpoldrack

FTR, I thought that may be it is https://github.com/datalad/datalad/issues/6623 but it is different test

https://ci.appveyor.com/project/mih/datalad/builds/44097916/job/8ctjnacmj36d10a0

../datalad/distributed/tests/test_create_sibling_ghlike.py::test_invalid_call create(ok): . (dataset)
An access token is required for https://gin.g-node.org. Visit https://gin.g-node.org/user/settings/applications to create a token
token: 

yarikoptic avatar Jul 08 '22 20:07 yarikoptic

what shall we do with this one? I checked the actions status at all commits in this PR, and they all show the failure. Does anyone remember if that failure occurred before, e.g., when https://github.com/datalad/datalad/commit/f95d1887d337bcfe2877c52f71c74ff677362bb6 was the only commit this PR (I think it was force-pushed over at some point)? I vaguely remember to see a green CI

adswa avatar Aug 18 '22 13:08 adswa

Nobody has worked on this for a month. There is virtually no cost to closing a PR to stop exploiting people's attention span. If interest ever rekindles, reopening is a button push. Tag 'stale' and close. No activity for a week or two should be enough of a threshold, because, again, there is no cost. And the CI that would need to rerun is a good thing.

And yes, feel free to close any and all of my PRs that match this too. I would support adding a bot doing this mercilessly.

mih avatar Aug 18 '22 20:08 mih

This is again inactive for more than a month. Let's close inactive drafts, ok?

mih avatar Nov 24 '22 08:11 mih

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

View more on Code Climate.

codeclimate[bot] avatar Nov 30 '22 16:11 codeclimate[bot]

Fixed the interactivity issue (it was the introduced test, not an actual code change), rebased and removed the disabling of parallel test execution again. Only remaining "failures" before that rebase were about test_add_archive_content (not actually failing the test, but complaining the test took to long).

bpoldrack avatar Dec 09 '22 12:12 bpoldrack

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

View more on Code Climate.

codeclimate[bot] avatar Dec 13 '22 13:12 codeclimate[bot]

Remaining failures are in macOS setup - see https://github.com/datalad/datalad/pull/7227

Actual code changes approved long ago - will go ahead and merge.

bpoldrack avatar Dec 14 '22 11:12 bpoldrack