Add smart punctuation #134
Conversation
|
Wow, thanks so much for taking this on! On first glance this looks pretty good. Would it be possible to split those fixes into separate commits? It would make it a little easier to visualize and manage those changes. I'll take a closer look later tonight or tomorrow and get back to you with any other thoughts or comments. Thanks again! |
Additional processors aren't able to access delimiters if they're removed by another processor first and bugs are introduced if delimiters aren't removed.
Done. Let me know if you need anything else! |
|
nice, so this basially implements #98 :) |
|
Oh good call @cebe, I forgot about that one. I think wrapping this up as an |
|
I've amended the most recent commit to remove an unnecessary line (adding a
I think it would make sense in the main repo, since it's part of jgm/commonmark.js. Were you thinking something along the lines of c2c9ec9? It is a barebones extension that, if used by itself, only parses punctuation and paragraphs. Otherwise, it can be added to another environment to inherit from there (as I updated the tests to do). Just let me know if you want to keep it or not and I can change/remove it as necessary. |
|
I have no strong opinion on whether this should be a separate package or not, it is not a heavy one and the JS implementation also ships it in one package so it may just stay in this repo. Am I right that this is not enabled by default? I am not that deep into the code to find that out. |
|
@bfrohs yes that is exactly what I had in mind! @cebe correct, this would not be enabled by default. And I agree with your point, perhaps it's best to just include this specific feature. I want to do one final read-through of the code tonight and then I will accept the merge. 😄 On a related side-note - I'm also wondering if perhaps we should look at reorganizing the classes a bit; basically separating the core parser, CommonMark elements/parsers, and this feature into 3 separate sub-namespaces. Right now things are all lumped in together. It's fine for now though - just something to think about in the future, especially if other similar features might be coming. |
|
@colinodell Fixed the latest commit per c2c9ec9#commitcomment-12068861 |
| // Process the emphasis characters | ||
| $delimiterStack->iterateByCharacters(['“', "’"], $callback, $stackBottom); | ||
| // Remove gaps | ||
| $inlines->removeGaps(); |
There was a problem hiding this comment.
Lines 43 and 44 can (should?) probably be removed. I'm thinking this was probably copypasted from the other processor which basically nulls-out some of the inlines to mark them for eventual removal by removeGaps(). We're not doing this here, so the removeGaps() call shouldn't be necessary.
There was a problem hiding this comment.
Yup, copy/paste issue. Fixed.
|
Alright I've taken a close look and am really happy with what you've done :) I've just got two final comments. Let me know your thoughts. |
May need to revert this depending on commonmark/commonmark.js#56
| @@ -23,6 +23,7 @@ | |||
| "require-dev": { | |||
| "erusev/parsedown": "~1.0", | |||
| "jgm/CommonMark": "0.20", | |||
| "jgm/SmartPunct": "0.20", | |||
There was a problem hiding this comment.
Why? The smart_punct.txt file is included in jgm/CommonMark.
There was a problem hiding this comment.
Why? The smart_punct.txt file is included in jgm/CommonMark.
Is it? jgm/CommonMark points to http://spec.commonmark.org/0.20/spec.txt in composer.json, which doesn't seem to include any smart punctuation tests. Additionally, there is no smart_punct.txt file in jgm/CommonMark anywhere I can find it.
There is, however a smart_punct.txt file in jgm/CommonMark.js, which is what jgm/SmartPunct points to in this file.
There was a problem hiding this comment.
I'm sorry, I didn't notice it.
|
Merged. Thanks so much for this contribution! |
Per commonmark/commonmark.js@7bd852b
This PR implements the smart punctuation work done in commonmark.js and fixes a few bugs with this package.