Skip to content

[ENH] Reorganise modality agnostic files section into multiple pages #2106

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

Merged
merged 18 commits into from
May 20, 2025

Conversation

bclenet
Copy link
Contributor

@bclenet bclenet commented Apr 23, 2025

BEP028 BIDS-Prov is going to introduce a lot of contents into the Modality agnostic files section of the BIDS specification.

In order to prepare for these changes, this PR splits the contents of the src/modality-agnostic-files.md file into separate files in a src/modality-agnostic-files/ directory:

  • src/modality-agnostic-files/dataset-description.md
  • src/modality-agnostic-files/data-description.md
  • src/modality-agnostic-files/code.md

@bclenet bclenet marked this pull request as ready for review April 23, 2025 10:21
Copy link

codecov bot commented Apr 25, 2025

Codecov Report

All modified and coverable lines are covered by tests ✅

Project coverage is 82.10%. Comparing base (1e14c26) to head (fb5a33a).

Additional details and impacted files
@@           Coverage Diff           @@
##           master    #2106   +/-   ##
=======================================
  Coverage   82.10%   82.10%           
=======================================
  Files          17       17           
  Lines        1531     1531           
=======================================
  Hits         1257     1257           
  Misses        274      274           

☔ View full report in Codecov by Sentry.
📢 Have feedback on the report? Share it here.

🚀 New features to boost your workflow:
  • ❄️ Test Analytics: Detect flaky tests, report on failures, and find test suite problems.
  • 📦 JS Bundle Analysis: Save yourself from yourself by tracking and limiting bundle sizes in JS merges.

@effigies effigies requested review from effigies and removed request for erdalkaraca, DimitriPapadopoulos and sappelhoff April 25, 2025 13:47
@bclenet
Copy link
Contributor Author

bclenet commented May 13, 2025

Hi !
CI seems fails (check_links) fails because of a link that is not related to the PR. Could someone please relaunch the CI pipeline so that this fail disappears ?
Thanks

@cmaumet
Copy link
Collaborator

cmaumet commented May 14, 2025

Hi everyone. Is it okay to merge this PR or are there more updates that would be required? As a note, this is only a refactoring of existing info (no new and/or no remove text). Thanks!

Copy link
Collaborator

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

I am a little uncertain about using "Data descriptions" here, since "Dataset" includes "Data" as well.
Especially since to a degree summaries like participants.tsv are also "dataset level description": although they already were separated from "Dataset description" section, they were not explicitly sectioned under "Data" section.

here is what is there:
image

It seems that what is common among all, but phenotype/, those "Data description" files -- they are directly pertinent to data under sub-* folders. phenotype/, sure thing, also but it is under a separate folder. I wonder if we should

@bclenet
Copy link
Contributor Author

bclenet commented May 15, 2025

Hi @yarikoptic thanks for the review, I changed the names of the "Data description" section and added a new one for Phenotypic and assessment data, as you suggested.

Copy link
Collaborator

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

IMHO looks nice and now more consistent with "Modality specific files" section being expandable too

image

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.

LGTM, apart from the redirects.

@effigies effigies changed the title [ENH][SPEC] Reorganise modality agnostic files section [ENH] Reorganise modality agnostic files section into multiple pages May 16, 2025
@cmaumet
Copy link
Collaborator

cmaumet commented May 20, 2025

Hey @yarikoptic @effigies are we good to merge with your two approvals or should we wait for more input? Sorry just not sure about the process here... Thanks!

@effigies
Copy link
Collaborator

I think we can merge. Been over a week since significant changes, just touch-ups since then.

@effigies effigies merged commit 0dd0d36 into bids-standard:master May 20, 2025
15 of 25 checks passed
@yarikoptic
Copy link
Collaborator

but ideally we are still to add more redirects whenever someone (might be us) brings that mkdocs/mkdocs-redirects#73 over the finish line

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.

4 participants