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] update highlighting of examples, JSON keys and values, and TSV headers or values in the schema #998

Merged
merged 5 commits into from
Feb 4, 2022

Conversation

Remi-Gau
Copy link
Collaborator

@Remi-Gau Remi-Gau commented Feb 3, 2022

closes #746

make sure that that examples appear "as they should" be in JSON or TSV files

  • numeric examples appear for example like this 1
  • JSON keys or values when appearing in metadata descriptions appear with double quotes for example like "Key" or "n/a"
  • TSV column header or value appear without double quotes for example like 1 or like this n/a

I am sure I have missed some but this is a never ending battle (and I don't thing we want to automate or CI check this) so I suggest that this should close #746

@@ -201,7 +201,7 @@ reconstruction:
entity: rec
description: |
The `rec-<label>` key/value can be used to distinguish
different reconstruction algorithms (for example ones using motion
different reconstruction algorithms (for example `MoCo` for the ones using motion
Copy link
Collaborator Author

Choose a reason for hiding this comment

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

This feels like common enough occurrence in scanners output to add it by name as an example

@codecov
Copy link

codecov bot commented Feb 3, 2022

Codecov Report

Merging #998 (7529d75) into master (2507964) will increase coverage by 1.07%.
The diff coverage is n/a.

Impacted file tree graph

@@            Coverage Diff             @@
##           master     #998      +/-   ##
==========================================
+ Coverage   34.05%   35.13%   +1.07%     
==========================================
  Files           8        8              
  Lines         834      834              
==========================================
+ Hits          284      293       +9     
+ Misses        550      541       -9     
Impacted Files Coverage Δ
tools/schemacode/schemacode/_version.py 38.90% <0.00%> (+2.59%) ⬆️

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 49b9f48...7529d75. Read the comment docs.

@@ -744,7 +744,7 @@ EffectiveEchoSpacing:
between lines in the phase-encoding direction,
defined based on the size of the reconstructed image in the phase direction.
It is frequently, but incorrectly, referred to as "dwell time"
(see `DwellTime` parameter below for actual dwell time).
(see `"DwellTime"` parameter for actual dwell time).
Copy link
Collaborator Author

Choose a reason for hiding this comment

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

removed the below because this might not make sense in all context if this description was used in a table where DwellTime does not appear

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.

Overall, I like these changes. Just two things:

  1. Including a leading underscore when referring to suffixes/specific files (e.g., _events.tsv) seems less readable to me.
  2. I'm not sure if you want to include quotes around fields when you're referring to the actual contents of the field, rather than the key. For example, in the RepetitionTimeExcitation description, we have:

Use RepetitionTimeExcitation (in combination with "RepetitionTimePreparation" if needed)

It seems like "RepetitionTimePreparation" should just be RepetitionTimePreparation in this case. That said, I don't feel strongly about this, so if you disagree then I'm happy to approve.

src/schema/objects/metadata.yaml Outdated Show resolved Hide resolved
src/schema/objects/metadata.yaml Outdated Show resolved Hide resolved
Co-authored-by: Taylor Salo <tsalo006@fiu.edu>
@Remi-Gau
Copy link
Collaborator Author

Remi-Gau commented Feb 3, 2022

Overall, I like these changes. Just two things:

1. Including a leading underscore when referring to suffixes/specific files (e.g., `_events.tsv`) seems less readable to me.

2. I'm not sure if you want to include quotes around fields when you're referring to the actual contents of the field, rather than the key. For example, in the `RepetitionTimeExcitation` description, we have:

Use RepetitionTimeExcitation (in combination with "RepetitionTimePreparation" if needed)

It seems like "RepetitionTimePreparation" should just be RepetitionTimePreparation in this case. That said, I don't feel strongly about this, so if you disagree then I'm happy to approve.

Yeah In a way I agree but this adds another implicit rule about formatting that will be hard to keep track off and potentially confuse users, no?

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.

That's a fair point. Approved!

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.

it seems like you removed the leading underscores again based on a comment by Taylor. I think you missed two instances (see comments).

I also like events.tsv better than _events.tsv even though I know I've been using both in the past

src/schema/objects/metadata.yaml Outdated Show resolved Hide resolved
src/schema/objects/metadata.yaml Outdated Show resolved Hide resolved
Co-authored-by: Stefan Appelhoff <stefan.appelhoff@mailbox.org>
@Remi-Gau
Copy link
Collaborator Author

Remi-Gau commented Feb 4, 2022

it seems like you removed the leading underscores again based on a comment by Taylor. I think you missed two instances (see comments).

I also like events.tsv better than _events.tsv even though I know I've been using both in the past

Thanks for catching those too!!

@sappelhoff sappelhoff merged commit e47283a into bids-standard:master Feb 4, 2022
Lestropie added a commit to Lestropie/bids-specification that referenced this pull request Apr 29, 2022
Resolves bids-standard#947 against:
- bids-standard#962 (21b7725)
- bids-standard#1044 (d9eeb6d)
- bids-standard#867 (5fe26e1)
- bids-standard#998 (e47283a)

Conflicts:
	src/02-common-principles.md
	src/schema/README.md
	src/schema/objects/entities.yaml
	src/schema/objects/metadata.yaml
@Remi-Gau Remi-Gau deleted the format_example branch August 17, 2022 07:23
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.

Remaining formatting issues in tables
3 participants