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

addurls: Provide better placeholder error for special keys #5664

merged 3 commits into from May 17, 2021


Copy link

@kyleam kyleam commented May 14, 2021

As described in gh-4658, addurls gives a confusing error message if it can't derive fields like _url_basename from the provided URL field. Check if a placeholder is a special key and give a more helpful message.

_get_placeholder_exception() callers pass in the row to provide a
collection of known keys, but an upcoming commit will care about
having access to the entire row.  Rename the parameter for clarity.
_get_placeholder_exception() builds up the exception message from
several parts, including a message prefix passed in by the caller.  As
a reader, it's a bit confusing to keep those parts in your head and
construct the final message.  Plus there's a confusing condition on
"sugmsg" even though it's never false.

Instead construct the entire message for each condition, with the
caller passing in what's being formatted ("URL" or "file name").
For each row, addurls() derives special keys from the URL.  If a row
is missing a special key (e.g., "_url_basename" because the URL has
not path), addurls() emits a confusing error:

  Unknown placeholder '_url_basename' in file name: Did you mean any
  of these?  _url_hostname

For _that_ row, '_url_basename' couldn't be derived and is unknown,
but that's unhelpful to say to the caller given that '_url_basename'
is a documented placeholder.

Instead check whether the key error is for a special key.  If it is,
say that the field couldn't be constructed, including the problematic
row in the message.

Closes datalad#4658.
Copy link

codecov bot commented May 14, 2021

Codecov Report

Merging #5664 (973e053) into maint (2c60c53) will decrease coverage by 0.01%.
The diff coverage is 100.00%.

Impacted file tree graph

@@            Coverage Diff             @@
##            maint    #5664      +/-   ##
- Coverage   90.31%   90.29%   -0.02%     
  Files         299      299              
  Lines       42312    42328      +16     
+ Hits        38214    38222       +8     
- Misses       4098     4106       +8     
Impacted Files Coverage Δ
datalad/plugin/ 97.07% <100.00%> (+0.02%) ⬆️
datalad/plugin/tests/ 99.54% <100.00%> (+0.01%) ⬆️
datalad/downloaders/tests/ 89.07% <0.00%> (-1.91%) ⬇️

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 2c60c53...973e053. Read the comment docs.

Copy link

@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.

On behalf of the UX testing group, thank you @kyleam ! ;-)

@kyleam kyleam merged commit b38b2b5 into datalad:maint May 17, 2021
@kyleam kyleam deleted the addurls-better-url-parts-error branch May 17, 2021 17:27
@yarikoptic yarikoptic added the semver-patch Increment the patch version when merged label Jun 2, 2021
@yarikoptic yarikoptic mentioned this pull request Jun 3, 2021
8 tasks
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
semver-patch Increment the patch version when merged
None yet

Successfully merging this pull request may close these issues.

None yet

2 participants