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

Trim excess dashes from heading IDs #457

Closed
wants to merge 1 commit into from

Conversation

Feder1co5oave
Copy link
Contributor

When I have something like

# Heading with some trailing punctuation...

With the default Renderer I get

<h1 id="heading-with-some-trailing-punctuation-">Heading with some trailing punctuation...</h1>

(notice the trailing dash in the id).
This PR fixes this:

<h1 id="heading-with-some-trailing-punctuation">Heading with some trailing punctuation...</h1>

Warning: this may change the way URLs are generated and break existing links.

@joshbruce
Copy link
Member

@Feder1co5oave: Would this fix the failing test related to header IDs??

@joshbruce joshbruce added this to the 0.4.0 - No known defects milestone Jan 21, 2018
@Feder1co5oave
Copy link
Contributor Author

Nope. In fact, with that test it would have the same effect as applying #456

@joshbruce
Copy link
Member

Bummer. Thinking about closing this then. Thoughts??

@Feder1co5oave
Copy link
Contributor Author

Uhm this has nothing to do with that. That issue arose because when writing test cases, I put in headings' id as they were generated by marked before merging #456 (to make them pass). The change I suggested here should have been included in 456, I guess.

@joshbruce
Copy link
Member

@Feder1co5oave: Just to make sure I'm picking up what you're putting down. Merging this won't fix the failing test. Merging this won't conflict with the #456 solution.

Having said that, I thought I saw a PR (other than #456) elsewhere talking about the trailing dash heading issue.

Now thinking about just merging this then. Trying to clean up PRs a bit.

@Feder1co5oave
Copy link
Contributor Author

Let's put it this way: both this and #456 target the default generation of id attributes for headings trying to improve it. My take on this subject is that if one wants to rely on headings ids, he should override the renderer in order to write a text transformation function that suits his needs. There is no perfect way to slugify a piece of text and turn it into an id. I'm starting to think that we should remove it as default behavior. But then users would complain about that, because ids are convenient, at times.

Commonmark does not even hint at id generation for headings. I guess GFM does, I haven't checked.

Where generating ids, there are problems about punctuation, non-ansi characters, DOM clobbering and so on, that trying to solve them all is beyond our capabilities. There exist some libraries (I'm thinking uslug) that do just that. We could try to replicate the id generation of github...

This pr tries to improve the default generation of id, but it doesn't solve any "real issues"

@joshbruce
Copy link
Member

@Feder1co5oave: I tend to agree. I haven't seen a Markdown spec that has id as part of the heading definitions (did a quick skim of GFM and didn't see it anywhere). So, I think GitHub is using some post-processor not their parser to accomplish id generation.

This is what I was going for in #919 as well. I think there would be complaints; however, I think the integrity of Marked would be better served with an extensible solution like you describe.

The core of Marked, imho, should be reserved for converting Markdown to HTML based on one or more specifications, with the option to extend. Having said that, one of the things I've always appreciated about Markdown is that it is not limited to converting to HTML; the more stuff we add that is HTMl specific the more it feels like we move away from this decoupling. Almost like we (Marked) are perpetuating the notion that Markdown is just a simpler way of writing HTML...instead of Markdown essentially being a markup language in and of itself.

That's my philosophical contribution for the day. ;)

Closing to be revisited at a later time. If we do something with the heading ids it will probably need to wait until 0.5.0 or later as we process the more pressing issue of spec compliance over new features.

@joshbruce joshbruce closed this Jan 22, 2018
@Feder1co5oave
Copy link
Contributor Author

@joshbruce no problem for me.

Should we open a dedicated issue to discuss default id generation?
Whether we should do it, how to do it, whether to replicate github behavior, how to take into account utf-8, how to make it easy to do it in a post-processing task...

See #981 (comment) for a collection of id-related issues.

@joshbruce
Copy link
Member

@Feder1co5oave: That sounds like a wonderful idea. I'm moving between meetings at the moment, can you take that up?

@joshbruce
Copy link
Member

The dedicated issue thing. :)

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