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

[FIX] wrong usage of DOI in SourceDatasets example #1361

Merged
merged 4 commits into from
Dec 22, 2022

Conversation

sappelhoff
Copy link
Member

xref: bids-standard/bids-examples#339

Correcting a BIDS-URI example --> relative paths for BIDS-URIs do not start with a file:// ... we use file:// only for absolute paths (such as in file:///home/stefanappelhoff/Downloads/myfile.nii)

I think we can consider this a bugfix.

cc @Remi-Gau @effigies

@sappelhoff
Copy link
Member Author

scratch that ... originally I wanted to fix this example (end of sourcedata section):

-{
-  "SourceDatasets": [ {"URL": "file://../../rawdata/"} ]
-}
+{
+  "SourceDatasets": [ {"URL": "../../rawdata/"} ]
+}

However then I realized that:

  1. this example is not about BIDS-URIs
  2. this example has been introduced into the derivatives branch 4 years ago

So we probably shouldn't change it. At least not without proper deprecation. I would be fine with repurposing this PR to doing the minor typo fix that I committed in ee02f1c

... but we can also use this space to discuss whether we find it problematic that:

  1. In BIDS-URIs, relative paths do not have a leading file://
  2. for SourceDatasets relative paths, this is recommended
  3. In all other places (I hope) we have deprecated relative paths in favor of BIDS-URIs

A proper fix may be to permit BIDS-URIs for SourceDatasets and to RECOMMEND using them when relative paths are in play. Much like permitting a field BIDS-URI next to the permitted fields URL, DOI, and Version for SourceDatasets. What do you think?

@sappelhoff sappelhoff changed the title [FIX] BIDS-URI example contained a faulty 'file://' in relative path [FIX] wrong usage of DOI in SourceDatasets example Nov 21, 2022
@codecov
Copy link

codecov bot commented Nov 21, 2022

Codecov Report

Base: 88.65% // Head: 88.65% // No change to project coverage 👍

Coverage data is based on head (a832a4b) compared to base (49cb321).
Patch has no changes to coverable lines.

❗ Current head a832a4b differs from pull request most recent head 4997261. Consider uploading reports for the commit 4997261 to get more accurate results

Additional details and impacted files
@@           Coverage Diff           @@
##           master    #1361   +/-   ##
=======================================
  Coverage   88.65%   88.65%           
=======================================
  Files          11       11           
  Lines        1084     1084           
=======================================
  Hits          961      961           
  Misses        123      123           

Help us with your feedback. Take ten seconds to tell us how you rate us. Have a feature suggestion? Share it here.

☔ View full report at Codecov.
📢 Do you have feedback about the report comment? Let us know in this issue.

@@ -259,7 +259,7 @@ field in `dataset_description.json` of each subdirectory of `derivatives` to:

```JSON
{
"SourceDatasets": [ {"URL": "file://../../rawdata/"} ]
"SourceDatasets": [ {"URL": "../../rawdata/"} ]
Copy link
Collaborator

Choose a reason for hiding this comment

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

According to 4.3. URL writing:

A relative-URL string must be one of the following, switching on base URL’s scheme:

Therefore, a relative URL cannot start with file://. Instead, a path-relative-scheme-less-URL like ../../rawdata/ is valid.

But then, I don't know if it works here...

@effigies
Copy link
Collaborator

effigies commented Dec 1, 2022

Per today's call: The fix is just to drop file:// when it shows up with relative paths. This is not a spec change, just a fix for examples.

@sappelhoff sappelhoff merged commit 2b69916 into bids-standard:master Dec 22, 2022
@sappelhoff sappelhoff deleted the fix/bidsuri branch December 22, 2022 12:04
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants