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

Control example order via explicit list, not via sorting titles #1321

Open
wants to merge 4 commits into
base: main
Choose a base branch
from

Conversation

drammock
Copy link
Member

PR Description

This PR removes numbering from the titles of the examples, and switches the example ordering heuristic from relying on the (no-longer-numbered) titles, to instead relying on a JSON file containing example filenames in a fixed order. Motivation is that:

  • there were two examples numbered 13
  • the second of the "13"s seemed out of place at the end, but inserting it in the middle would have required renumbering all titles that came after its new place.

Safeguards:

  • If a new example is created and not added to the JSON file, it will automatically sort last (not be omitted from the build).
  • If an example's filename changes without updating the JSON file, it will sort last (not be omitted from the build).
  • Recovering from either of the above situations is a one-line change to the JSON file (i.e. adding the new filename / updating the old filename to its new name).

closes #1320

Merge checklist

Maintainer, please confirm the following before merging.
If applicable:

  • All comments are resolved
  • This is not your own PR
  • All CIs are happy
  • PR title starts with [MRG]
  • whats_new.rst is updated
  • New contributors have been added to CITATION.cff
  • PR description includes phrase "closes <#issue-number>"

Copy link
Member Author

@drammock drammock left a comment

Choose a reason for hiding this comment

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

quick self-review to call out / fix a couple things

"rename_brainvision_files.py",
"convert_mri_and_trans.py",
"convert_ieeg_to_bids.py",
"convert_nirs_to_bids.py",
Copy link
Member Author

Choose a reason for hiding this comment

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

this is the one that was previously a duplicate "13" and appearing last. I thought it made more sense to group it with other "converting XXX to BIDS" examples.

Aside: if anyone has strong feelings about order / wants to take this opportunity to shuffle the order a bit more, feel free to push a commit changing this file.

Comment on lines 522 to 524
# better example sorting, without relying on numbers in example titles
with open(Path(__file__).parents[1] / "doc" / "example_order.json") as fid:
EXAMPLE_ORDER = json.load(fid)
Copy link
Member Author

Choose a reason for hiding this comment

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

NB: the JSON file won't be included in the built wheel, but that shouldn't matter as it's only needed when building our docs, so we can assume a clone of the repo in that case.

mne_bids/utils.py Outdated Show resolved Hide resolved
mne_bids/utils.py Outdated Show resolved Hide resolved
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.

Great, thanks a lot Dan. I like the changes, +1 to merge once CIs are green.

@drammock
Copy link
Member Author

just a note that this needs sphinx-gallery/sphinx-gallery#1391 before the CIs will pass.

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.

there are 2 examples numbered "13"
2 participants