-
Notifications
You must be signed in to change notification settings - Fork 75
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
🥼 Improve suffix citation parsing #757
Conversation
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Seems good to me!
@@ -25,7 +25,8 @@ export interface Citation { | |||
export const citationsPlugin: PluginWithOptions = (md) => { | |||
const regexes = { | |||
citation: /^([^^-]|[^^].+?)?(-)?@([\w][\w:.#$%&\-+?<>~/]*)(.+)?$/, | |||
inText: /^@((?:[\w|{][\w:.#$%&\-+?<>~/]*[\w|}])|\w)(\s*)(\[)?/, | |||
// Only allow a short [suffix] for in text citations (e.g. 50 characters) | |||
inText: /^@((?:[\w|{][\w:.#$%&\-+?<>~/]*[\w|}])|\w)(\s*)(\[([^\]]{1,50})\])?/, |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
So this regex includes the suffix, while the previous did not. That means we are no longer using parseLinkLabel
and instead just get the suffix up front as raw text. Probably ok? Certainly makes the code simpler!
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Yeah, the parse link label was taking a lot of content (including other citations and links) and I think we want to be pretty targeted here and ensure that we can close the suffix label properly!
- type: text | ||
content: 'These include experimentally produced alkaline magmas from ' | ||
- type: cite | ||
content: '@iacovino2016 [`alkaline.xlsx`]' |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Running this test before your change - this content
was enormous! Now it looks nice. 👍
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
🚀
The current version will eagerly parse too many labels including if we turn some of the nesting off. This improvement excludes
[
, uses a regexp, and ensures that it is short (50 characters).