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

UX: When two or more clone URL templates are found, error out more gracefully #5839

Merged
merged 3 commits into from
Aug 3, 2021

Conversation

adswa
Copy link
Member

@adswa adswa commented Jul 29, 2021

This is cherry-picked from #5644 because it fixes an issue unrelated to that PR. If someone ends up with two configurations for source candidates with identical names, one would see

❱ datalad get HCP1200/100206                                                1 !
[ERROR  ] 'tuple' object has no attribute 'format' [get.py:_get_flexible_source_candidates_for_submodule:240] (AttributeError) 

With this change, a user gets a more helpful error message:

❱ datalad get HCP1200/100206                                                2 !
[ERROR  ] There are multiple URL templates for submodule clone candidate 'origin', but only one is allowed. Check your configuration! [get.py:_get_flexible_source_candidates_for_submodule:245] (ValueError) 

@codecov
Copy link

codecov bot commented Jul 29, 2021

Codecov Report

Merging #5839 (24f7a18) into maint (7f543d0) will decrease coverage by 5.38%.
The diff coverage is 100.00%.

❗ Current head 24f7a18 differs from pull request most recent head 7ba9e64. Consider uploading reports for the commit 7ba9e64 to get more accurate results
Impacted file tree graph

@@            Coverage Diff             @@
##            maint    #5839      +/-   ##
==========================================
- Coverage   90.28%   84.89%   -5.39%     
==========================================
  Files         300      297       -3     
  Lines       42370    42341      -29     
==========================================
- Hits        38252    35946    -2306     
- Misses       4118     6395    +2277     
Impacted Files Coverage Δ
datalad/distribution/get.py 96.26% <100.00%> (-1.65%) ⬇️
datalad/distribution/tests/test_get.py 81.23% <100.00%> (-18.77%) ⬇️
datalad/plugin/addurls.py 94.84% <100.00%> (-0.69%) ⬇️
datalad/plugin/tests/test_addurls.py 99.54% <100.00%> (+<0.01%) ⬆️
datalad/support/tests/utils.py 0.00% <0.00%> (-100.00%) ⬇️
datalad/metadata/extractors/tests/test_audio.py 19.35% <0.00%> (-80.65%) ⬇️
datalad/metadata/extractors/xmp.py 12.96% <0.00%> (-79.63%) ⬇️
datalad/metadata/extractors/exif.py 18.75% <0.00%> (-78.13%) ⬇️
datalad/metadata/extractors/image.py 19.35% <0.00%> (-77.42%) ⬇️
datalad/metadata/extractors/audio.py 20.00% <0.00%> (-77.15%) ⬇️
... and 100 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 7f543d0...7ba9e64. Read the comment docs.

@adswa
Copy link
Member Author

adswa commented Jul 29, 2021

oh no the semverlabel check is here...

@adswa adswa added the semver-internal Changes only affect the internal API label Jul 29, 2021
@yarikoptic yarikoptic added semver-patch Increment the patch version when merged and removed semver-internal Changes only affect the internal API labels Jul 29, 2021
@yarikoptic
Copy link
Member

Looks good to me @adswa and thank you for the test! I changed label from internal (for internal changes/tools/utils some CI setup, which aren't delivered to the user) to patch (e.g. a fix to a bug), to trigger CI fail, only to realize that

  • my desires I expressed to @jwodder to ensure that master always has minor might be flawed in case of submitting a PR fixing a bug introduced in master and not yet released
  • that this PR might actually better fit to be for maint branch as what it is, isn't it? ;-)

@yarikoptic
Copy link
Member

eh, so we are starting to hit limits on how many OSX builders are available to github actions:

2021-07-29T15:17:38.9242652Z The agent pool assigned to this job has hit their MacOs concurrency limits
2021-07-29T15:17:38.9475063Z Found online and idle hosted runner(s) in the current repository's organization account that matches the required labels: 'macos-latest'
2021-07-29T15:17:38.9475164Z Waiting for a hosted runner in 'organization' to pick this job...

as for semver, just pushed directly to maint and master allowing for patch within master branch. See 7f543d0 . But still -- this PR might be better of to target maint

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.

Great, thx. I hope it would not matter if this ends up in maint or master. I am fine with either choice.

datalad/distribution/get.py Outdated Show resolved Hide resolved
…acefully

Previously, when someone would specify submodule clone candidates under the same name, we would crash
with an Attribute error. This is caught earlier now, fixing datalad#5715, and the problematic candidate name
is reported to the user
@adswa adswa changed the base branch from master to maint August 2, 2021 13:10
@adswa
Copy link
Member Author

adswa commented Aug 2, 2021

rebased to maint 👍

@adswa
Copy link
Member Author

adswa commented Aug 3, 2021

A single failing test:

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

FAIL: datalad.tests.test_witless_runner.test_asyncio_forked

----------------------------------------------------------------------

Traceback (most recent call last):

  File "/tmp/dl-miniconda-60pqdy8v/lib/python3.9/site-packages/nose/case.py", line 198, in runTest

    self.test(*self.arg)

  File "/tmp/dl-miniconda-60pqdy8v/lib/python3.9/site-packages/datalad/tests/utils.py", line 764, in _wrap_with_tempfile

    return t(*(arg + (filename,)), **kw)

  File "/tmp/dl-miniconda-60pqdy8v/lib/python3.9/site-packages/datalad/tests/test_witless_runner.py", line 221, in test_asyncio_forked

    raise AssertionError("Child process did not create a file we expected!")

AssertionError: Child process did not create a file we expected!

have never seen this one before

@mih
Copy link
Member

mih commented Aug 3, 2021

That test failure is unrelated.

Co-authored-by: Michael Hanke <michael.hanke@gmail.com>
raise ValueError(
f"There are multiple URL templates for submodule clone "
f"candidate '{name}', but only one is allowed. "
f"Check datalad.get.subdataset-source-candidate-* configuration!"
Copy link
Member

Choose a reason for hiding this comment

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

just a note -- (although I like consistency of f" prefix too) - f" is not needed on the lines where no {format} was used

Copy link
Member Author

Choose a reason for hiding this comment

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

ha, good to know! thanks!

Copy link
Member

@yarikoptic yarikoptic left a comment

Choose a reason for hiding this comment

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

If no surprises from the tests -- looks good to me, thanks @adswa

@yarikoptic yarikoptic added release Create a release when this pr is merged UX user experience labels Aug 3, 2021
@yarikoptic
Copy link
Member

whenever tests pass, lets merge to release (added a label)

@yarikoptic
Copy link
Member

appveyor -- macs + ubuntu freaked out

git clone -q https://github.com/datalad/datalad.git /home/appveyor/projects/datalad
git fetch -q origin +refs/pull/5839/merge:
git checkout -qf FETCH_HEAD
A WebException with status SendFailure was thrown.

Let's proceed

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
release Create a release when this pr is merged semver-patch Increment the patch version when merged UX user experience
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

4 participants