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

[ENH] use schema to mention which "top directories" are allowed #1289

Merged
merged 18 commits into from
Oct 18, 2022

Conversation

Remi-Gau
Copy link
Collaborator

@Remi-Gau Remi-Gau commented Sep 16, 2022

closes #1284

  • update macro doc
  • add test

Sorry, something went wrong.

Verified

This commit was created on GitHub.com and signed with GitHub’s verified signature.
@codecov
Copy link

codecov bot commented Sep 16, 2022

Codecov Report

Base: 88.33% // Head: 88.39% // Increases project coverage by +0.06% 🎉

Coverage data is based on head (4da60fa) compared to base (1053ce4).
Patch coverage: 100.00% of modified lines in pull request are covered.

Additional details and impacted files
@@            Coverage Diff             @@
##           master    #1289      +/-   ##
==========================================
+ Coverage   88.33%   88.39%   +0.06%     
==========================================
  Files          11       11              
  Lines        1080     1086       +6     
==========================================
+ Hits          954      960       +6     
  Misses        126      126              
Impacted Files Coverage Δ
...ools/schemacode/bidsschematools/render/__init__.py 100.00% <ø> (ø)
tools/schemacode/bidsschematools/render/text.py 97.47% <100.00%> (+0.07%) ⬆️

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.

@Remi-Gau
Copy link
Collaborator Author

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

Another would be useful would be to add internal links to the sections of specs that relate to each directory.

@VisLab
Copy link
Member

VisLab commented Sep 17, 2022 via email

@Remi-Gau
Copy link
Collaborator Author

Another would be useful would be to add internal links to the sections of specs that relate to each directory.

Done

@effigies
Copy link
Collaborator

@Remi-Gau Is this high priority, as in you want to get it in for the release? Trying to decide when to review this.

@Remi-Gau
Copy link
Collaborator Author

not high priority IMHO
@VisLab do you agree?

@VisLab
Copy link
Member

VisLab commented Sep 20, 2022 via email

Copy link
Member

@tsalo tsalo left a comment

Choose a reason for hiding this comment

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

It would be nice to ultimately have all of the info about each folder in the schema, so we don't need to link out to specific sections, but I think linking is fine for now. I just have one small suggestion.

EDIT: Actually... maybe it would look better as a table?

src/schema/objects/associated_data.yaml Outdated Show resolved Hide resolved
@Remi-Gau
Copy link
Collaborator Author

Remi-Gau commented Sep 22, 2022

I think that anyway @effigies has a pr in the pipeline (#1288 ) that will wreck a lot of how this work even if this does not change the final output so I am waiting for this one to be in and adapt from there.

@sappelhoff sappelhoff marked this pull request as draft September 27, 2022 10:13
@sappelhoff
Copy link
Member

I think that anyway effigies has a pr in the pipeline that will wreck a lot of how this work even if this does not change the final output so I am waiting for this one to be in and adapt from there.

converting to draft then -- please link to that "PR in the pipeline" if you know where it exists :)

@Remi-Gau
Copy link
Collaborator Author

please link to that "PR in the pipeline" if you know where it exists :)

done

@effigies
Copy link
Collaborator

@Remi-Gau Conflicting #1288 has been merged.

@Remi-Gau
Copy link
Collaborator Author

@Remi-Gau Conflicting #1288 has been merged.

thanks for the ping

will update accordingly

@Remi-Gau Remi-Gau marked this pull request as ready for review October 10, 2022 21:41
@Remi-Gau
Copy link
Collaborator Author

Still need to add test but other than that we are back on track.

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.

+1 to merge, pending the test.

@Remi-Gau
Copy link
Collaborator Author

sometimes use sub-directories and other times subdirectories in the spe

Should we do a separate PR for this?

@sappelhoff
Copy link
Member

Should we do a separate PR for this?

yes, I can do it, do you agree to always use the hyphen?

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.

Mostly aesthetic comments.

src/schema/objects/files.yaml Outdated Show resolved Hide resolved
src/02-common-principles.md Outdated Show resolved Hide resolved
tools/schemacode/bidsschematools/render/text.py Outdated Show resolved Hide resolved
Remi-Gau and others added 2 commits October 17, 2022 16:00
Co-authored-by: Chris Markiewicz <[email protected]>
Co-authored-by: Chris Markiewicz <[email protected]>
Copy link
Member

@tsalo tsalo left a comment

Choose a reason for hiding this comment

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

LGTM. Thanks!

@VisLab
Copy link
Member

VisLab commented Oct 18, 2022

@Remi-Gau Thanks for seeing this through.

@sappelhoff sappelhoff merged commit e437d73 into bids-standard:master Oct 18, 2022
@Remi-Gau Remi-Gau deleted the top_dir branch December 6, 2022 21:37
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.

What directories can be at the top level of a BIDS Dataset?
5 participants