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] URI "definition" and recommendation #629

Merged
merged 16 commits into from
Oct 7, 2020

Conversation

Remi-Gau
Copy link
Collaborator

@Remi-Gau Remi-Gau commented Oct 1, 2020

TO DO

  • add URI definition after the "File Formation specification" section
  • make sure DOIs in the specs (in json examples or otherwise) have the format doi:<path>

For reviewers

  • is this the right "place" for that definition?
  • anything else but the DOIs that should change?

Copy link
Member

@sappelhoff sappelhoff left a comment

Choose a reason for hiding this comment

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

anything else but the DOIs that should change?

IMHO all JSON fields that contain URLs or DOIs. For example CogAtlasID, which says:

image

source: table in https://bids-specification.readthedocs.io/en/latest/04-modality-specific-files/03-electroencephalography.html#sidecar-json-_eegjson

the big benefit would be that we can validate for URIs, see: bids-standard/bids-validator#1072

the drawback would be that probably so far people have been specifying DOIs either as a URL, or potentially using just the DOI without any prefixes.

But I could live with that drawback in order to make the spec better in the future. Could be considered a breaking change though 🤷‍♂️

src/02-common-principles.md Outdated Show resolved Hide resolved
src/02-common-principles.md Outdated Show resolved Hide resolved
@Remi-Gau
Copy link
Collaborator Author

Remi-Gau commented Oct 2, 2020

This looks ready to go. Some changes are I think not problematic but some other I am much less sure about especially in the "higher level" files ("common principles" and "modality agnostic") as they could "break" things.

Will add comments through the review panel.

Once again will leave the linting as a last step when we agree content.

src/02-common-principles.md Outdated Show resolved Hide resolved
@Remi-Gau
Copy link
Collaborator Author

Remi-Gau commented Oct 2, 2020

One last question: that relates to the diversity of format we use to refer to papers in the specs themselves.

There are a couple of links to some papers in the specs that use a URL rather than a DOI. Should those be changed?

See here:
https://bids-specification.readthedocs.io/en/stable/99-appendices/08-coordinate-systems.html#introduction

For example, should this:

(see e.g.,
[https://doi.org/10.1016/j.neuroimage.2012.01.024](https://doi.org/10.1016/j.neuroimage.2012.01.024))

be turned into this:

(see e.g.,
[doi:j.neuroimage.2012.01.024](https://doi.org/10.1016/j.neuroimage.2012.01.024))

@sappelhoff
Copy link
Member

There are a couple of links to some papers in the specs that use a URL rather than a DOI. Should those be changed?

I think that'd be reasonable. And I think @yarikoptic would agree as well. See his comment: #403 (comment)

fix linter


fix internal link target
@Remi-Gau
Copy link
Collaborator Author

Remi-Gau commented Oct 2, 2020

This one has been rebased and conflicts have been resolved.

So this should be ready to merge before we can move on to #628 for more of the same (and potentially #636 ?)

Copy link
Member

@sappelhoff sappelhoff left a comment

Choose a reason for hiding this comment

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

LGTM!

However, we need other people to chime in about the potential breakage of now wanting our DOIs specified as URI in the from doi:thedoi

From a technical validation perspectives, only the datasets will break that specified the DOI as just thedoi, because those that used https://doi.org/thedoi still have a valid URI.

here's the link to previous discussion on this: #629 (comment)

src/03-modality-agnostic-files.md Outdated Show resolved Hide resolved
src/03-modality-agnostic-files.md Outdated Show resolved Hide resolved
Remi-Gau and others added 2 commits October 2, 2020 20:15
Co-authored-by: Stefan Appelhoff <stefan.appelhoff@mailbox.org>
Copy link
Collaborator

@effigies effigies left a comment

Choose a reason for hiding this comment

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

This looks great! I have made some suggestions to maintain backwards compatibility. Let me know what you think.

src/02-common-principles.md Outdated Show resolved Hide resolved
src/02-common-principles.md Outdated Show resolved Hide resolved
src/03-modality-agnostic-files.md Outdated Show resolved Hide resolved
src/03-modality-agnostic-files.md Outdated Show resolved Hide resolved
src/02-common-principles.md Outdated Show resolved Hide resolved
src/03-modality-agnostic-files.md Outdated Show resolved Hide resolved
Remi-Gau and others added 2 commits October 3, 2020 18:05
Co-authored-by: Chris Markiewicz <effigies@gmail.com>
Apply suggestions from code review

Co-authored-by: Chris Markiewicz <effigies@gmail.com>
@Remi-Gau Remi-Gau force-pushed the remi-URI branch 2 times, most recently from 905c229 to 4ddf5a7 Compare October 5, 2020 08:04
lint table


fix link to deprecated section
Copy link
Collaborator

@effigies effigies left a comment

Choose a reason for hiding this comment

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

I'd still like to have a general note with the URI definition, but I can be argued out of it if you're so inclined.

src/02-common-principles.md Show resolved Hide resolved
src/03-modality-agnostic-files.md Show resolved Hide resolved
Remi-Gau and others added 2 commits October 5, 2020 17:01
@sappelhoff sappelhoff merged commit a447ace into bids-standard:master Oct 7, 2020
@sappelhoff
Copy link
Member

Thanks @Remi-Gau

@yarikoptic
Copy link
Collaborator

There are a couple of links to some papers in the specs that use a URL rather than a DOI. Should those be changed?

I think that'd be reasonable. And I think @yarikoptic would agree as well. See his comment: #403 (comment)

yes, I do. Thank you! ;)

@Remi-Gau Remi-Gau deleted the remi-URI branch November 15, 2021 05:30
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.

consistent "URI", "DOI", "URL": guidance on how to ensure an unambiguous link
5 participants