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

Support (balanced) nested parentheses in link destinations #166

Merged
merged 1 commit into from
Dec 1, 2016

Conversation

kivikakk
Copy link
Contributor

@kivikakk kivikakk commented Dec 1, 2016

Hey @jgm!

This PR changes the spec and parser to allow any number of balanced nested parentheses in link destinations; this turns out to be a pretty common thing in e.g. URLs to graph services, and the single parenthesis-pair restriction currently present feels somewhat arbitrary.

/cc @vmg

@kivikakk kivikakk mentioned this pull request Dec 1, 2016
@vmg
Copy link
Contributor

vmg commented Dec 1, 2016

@jgm: 👋

We're trying really hard to stick to the CommonMark spec, but while rolling the new parser to production, we've found there's an overwhelming amount of links that break when users paste them into a Markdown document.

These links mostly come from HTTP APIs that take parameters containing queries or math operations, and hence contain nested parentheses which are not escaped. This kind of links break when inserted into Markdown link syntax [link](url) because of the limitation on just one level on nesting.

The most frequent cases are Graphite query links and Mixpanel analytics links. Mixpanel, for instance, provides users directly with Markdown syntax to paste on their READMEs, and this syntax contains nested parenthesis, which now break in GitHub.

It's not feasible for us to reach out to all the services that generate Markdown and ask them to escape their parens in links. So that's why we'd like to propose this small change to spec, which we're already running in production. The code changes are minimal, and I think the resulting Markdown is significantly more user-friendly this way.

I'm not sure of what is the process to upstream the actual change to the Spec, but we've started with the change to the actual parser to prove its viability. Would you like me to propose the change on the Spec repo instead?

Thank you!

@jgm
Copy link
Member

jgm commented Dec 1, 2016 via email

@jgm jgm merged commit a9da678 into commonmark:master Dec 1, 2016
@jgm
Copy link
Member

jgm commented Dec 1, 2016

This looks ok to me. I'll port it over to jgm/CommonMark, the spec itself.

@vmg
Copy link
Contributor

vmg commented Dec 1, 2016

🙇

Rebasing our fork now. Thank you so much @jgm!

@kivikakk kivikakk deleted the kivikakk/nested-parens-upstream branch December 1, 2016 09:55
jgm added a commit to commonmark/commonmark-spec that referenced this pull request Dec 1, 2016
Also, note that the changes to the spec are thanks to
@kivikakk.
@kivikakk
Copy link
Contributor Author

kivikakk commented Dec 1, 2016

Thank you! And sorry if my initial PR was overly lacking in context or description; thanks @vmg for taking the time to explain the motivation better.

@jgm
Copy link
Member

jgm commented Dec 1, 2016

I don't think there was any motivation for the original limitation to one level of nesting beyond my (lazy) desire to parse it with a regex. So this is a good change.

CyberShadow pushed a commit to CyberShadow/cmark that referenced this pull request Apr 1, 2021
warning C4311: 'type cast': pointer truncation from 'void *' to 'int'
Fixes github#165
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.

4 participants