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

[INFRA] Move entity definitions to a separate page #568

Merged
merged 16 commits into from
Sep 8, 2020

Conversation

tsalo
Copy link
Member

@tsalo tsalo commented Aug 10, 2020

Closes #567.

Changes proposed:

  1. Add a new appendix for entity definitions, using bids_schema.py.
  2. Link to appendix entity definitions in the entity table, using bids_schema.py.
  3. Remove section headers for entity definitions in main text, to ensure that the search bar will point to the entities appendix page.
  4. Add links from main text to appendix definitions.
  5. Add a new function and command line subparser to bids_schema.py to generate the new appendix page.

Notes:

  • I didn't include Derivatives in this, but that might be something to do in the future. It will involve (1) adding derivatives-specific entities to the appendix (or a separate appendix), (2) supporting derivatives in the schema, and (3) adding links to the appendix in the Derivatives sections of the text.
  • The specification text is largely unchanged, but we will want to start removing entity definitions in the text, while leaving anything that provides necessary context for different modalities, such as examples.

@tsalo
Copy link
Member Author

tsalo commented Aug 10, 2020

It doesn't look like internal links work in the PDF, so we may need a workaround for that (even once we have the schema).

EDIT: Per maintainers call, this can be considered a web-only feature. It's not a big deal if the PDF cannot support these links, even though it should be possible with some work.

@tsalo tsalo mentioned this pull request Aug 10, 2020
5 tasks
@tsalo
Copy link
Member Author

tsalo commented Aug 28, 2020

@sappelhoff I switched to just linking from the text to the new page and dropping the section headers in the text. What do you think of the new structure?

Ultimately I'd like to minimize that "glue" (i.e., the section-specific entity descriptions) while maintaining readability and useful section-specific information (like examples), but I think this is a good start.

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.

nice, this looks like a less invasive start to me, which we can iterate upon in the future.

dropping the section headers in the text

why did you drop those?

@tsalo
Copy link
Member Author

tsalo commented Aug 29, 2020

I dropped the section headers for two reasons. First, the section headers only appeared for the first definition of each entity, setting that first definition up as the canonical one. If every data type section had subsections (with headers) for all of its applicable entities, I would have left it. Now, however, the main definition for each entity is in the Entities page, and each entity-specific text throughout the rest of the specification should slowly be downsized to just reflect additional information for that data type specifically. The second reason is that the section headers in the Entities page should be the ones that show up in the search bar.

@tsalo tsalo marked this pull request as ready for review September 1, 2020 18:29
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.

I think the proposed changes are a valuable change to the specification. Thanks for preparing it @tsalo - please tag some more people you'd like to review this (or write them an email, as some may not check their notifications)

@tsalo
Copy link
Member Author

tsalo commented Sep 3, 2020

Thanks @sappelhoff! Pinging @yarikoptic and @effigies.

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.

This is just wonderful, thank you @tsalo !

One little comment: I would have added a comment (probably just HTMLs') on top of entities and entities table markdown files:

<!-- 
  This file is autogenerated based on the src/schema.  DO NOT EDIT DIRECTLY. 
  Follow https://github.com/bids-standard/bids-specification/blob/master/CONTRIBUTING.md#updating-the-schema 
-->

@yarikoptic
Copy link
Collaborator

EDIT: Per maintainers call, this can be considered a web-only feature. It's not a big deal if the PDF cannot support these links, even though it should be possible with some work.

IMHO worth an issue (if not there already) so someone eager could just make it resolved! Would be a great addition to PDF rendering.

@tsalo
Copy link
Member Author

tsalo commented Sep 3, 2020

One little comment: I would have added a comment (probably just HTMLs') on top of entities and entities table markdown files:

<!-- 
  This file is autogenerated based on the src/schema.  DO NOT EDIT DIRECTLY. 
  Follow https://github.com/bids-standard/bids-specification/blob/master/CONTRIBUTING.md#updating-the-schema 
-->

That is a great idea! I'll add this to the entities and entity table pages. I'm just putting it in the intro text for now, but it could be built into its own function at some point.

I'll open an issue about the internal linking.

@tsalo
Copy link
Member Author

tsalo commented Sep 4, 2020

Thanks @sappelhoff and @yarikoptic. I'll wait the five days (starting yesterday), in case anyone wants to raise any concerns, before merging.

@yarikoptic Absolutely, the text could be moved out into a separate function, although I want to hold off on actually implementing that because I think the current code is going to be substantially restructured in the not-too-distant future.

@tsalo
Copy link
Member Author

tsalo commented Sep 8, 2020

The five day embargo period is over with no concerns raised, so I'm merging now. Thank you @sappelhoff and @yarikoptic!

@tsalo tsalo merged commit 751d177 into bids-standard:master Sep 8, 2020
@tsalo tsalo deleted the enh/entity-definitions branch September 8, 2020 15:15
@tsalo tsalo added schema-code Updates or changes to the code used to parse, filter, and render the schema. schema Issues related to the YAML schema representation of the specification. Patch version release. labels Apr 11, 2022
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
schema Issues related to the YAML schema representation of the specification. Patch version release. schema-code Updates or changes to the code used to parse, filter, and render the schema.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Move entity definitions to separate page(s)
3 participants