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

🧭 Order abbreviations by length #830

Merged
merged 4 commits into from
Jan 10, 2024
Merged

🧭 Order abbreviations by length #830

merged 4 commits into from
Jan 10, 2024

Conversation

agoose77
Copy link
Contributor

@agoose77 agoose77 commented Jan 8, 2024

This might not be the right place, or the best way to do this, but this PR orders abbreviations defined in the frontmatter by length (then by locale). As such, we no longer need to warn about the ordering.

@agoose77 agoose77 marked this pull request as draft January 8, 2024 16:25
@agoose77 agoose77 requested a review from rowanc1 January 8, 2024 16:38
@agoose77 agoose77 marked this pull request as ready for review January 8, 2024 16:38
Copy link
Collaborator

@rowanc1 rowanc1 left a comment

Choose a reason for hiding this comment

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

This looks awesome, thanks @agoose77. 🚀

Can we move the sort code down a level so it is closer to where we do some other filtering? This should then be able to be shared a bit easily as the CLI code isn't going to be easy to share, e.g. with jupyterlab-myst and others.

Could you also run npm run changeset and write a short message that bumps the myst-transform and myst-cli packages (the myst-cli one will likely be manual once the changes are pushed down). This helps keep track of which packages need bumping.

packages/myst-cli/src/process/mdast.ts Outdated Show resolved Hide resolved
@agoose77
Copy link
Contributor Author

agoose77 commented Jan 9, 2024

@rowanc1 thanks! I realised that my understanding of what the transform did was not quite right; initially I thought it wasn't sensible to move the logic to myst-transforms.

As such, I've moved this to a simple in-line sort, making this PR very small!

@rowanc1 rowanc1 changed the title feat: order abbreviations by length 🧭 Order abbreviations by length Jan 10, 2024
@rowanc1 rowanc1 merged commit d2e6e78 into main Jan 10, 2024
4 checks passed
@rowanc1 rowanc1 deleted the agoose77/feat-order-abbr branch January 10, 2024 01:19
@rowanc1
Copy link
Collaborator

rowanc1 commented Jan 10, 2024

Thanks again @agoose77, excited to see these sorts of quality-of-life changes come to life! :)

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.

2 participants