commonmark / commonmark.js

CommonMark parser and renderer in JavaScript
Other
1.44k stars 226 forks source link

Definitions of `can_open` and `can_close` #30

Closed elibarzilay closed 8 years ago

elibarzilay commented 9 years ago

This is not really an issue -- more like an extended comment, which could be used to simplify the code and/or the definitions if you think it's better. (Posting it here though it could be a comment on the spec or probably the cmark code.)

On first reading, I found the definitions of left/right flanking and their use in can-open/close very confusing. My guess is that initially the flanking concept was simplifying things, but IMO it's not as helpful now when more conditions are piled up. With the recent additional change I resorted to a drawing karnaugh maps, and the result is much shorter:

var sp_after  = reWhitespaceChar.test(char_after);
var sp_before = reWhitespaceChar.test(char_before);
var pn_after  = rePunctuation.test(char_after);
var pn_before = rePunctuation.test(char_before);
can_open  = !sp_after  && (pn_before || sp_before || (cc===C_ASTERISK && !pn_after ));
can_close = !sp_before && (pn_after  || sp_after  || (cc===C_ASTERISK && !pn_before));

The length is of course not too relevant, and I'm guessing that the speed would be practically the same. What seems important to me, and this might be just me, is that it's much easier to read, and makes more easily sense as a definition (had it been in the text).

iamstarkov commented 9 years ago

@elibarzilay sorry, I’m new to commonmark, can I ask you to introduce me the flanking concept? didn’t get it and related problem

jgm commented 9 years ago

+++ Vladimir Starkov [Apr 29 15 08:41 ]:

[1]@elibarzilay sorry, I’m new to commonmark, can I ask you to introduce me the flanking concept? didn’t get it and related problem

The flanking concept comes from the spec: http://spec.commonmark.org/0.19/#delimiter-run

The current commonmark.js code mirrors the spec, which is obviously a good thing. If the logic can be simplified, we might want to consider simplifying the spec. But I think it's better, either way, if the logic in commonmark.js mirrors the logic of the spec, so it's easier to confirm that it's correct.

elibarzilay commented 9 years ago

@jgm, well it's confirmed as much as I can: (a) I got to it via mechanic transformations of the original set of conditions, and (b) it passes all of the relevant spec tests. Should I file it there, or maybe it should go in the discussion thing?

jgm commented 8 years ago

@elibarzilay for the record, I can confirm that you're right about these reductions. Still, I think we'll keep the organizing concept of left- and right-flanking delimiter runs. After a bit of thinking how the spec might be rewritten, it still seems to me useful to do it this way.

elbarzil commented 8 years ago

@jgm, TBH, my motivation was the code... If this is the same (still, I haven't followed up on changes recently), then for the text it might be useful to show it as an alternative? (Possibly only if it can be added as some test somewhere, so if the main text changes this gets synced or removed...)

jgm commented 8 years ago

+++ Eli Barzilay [Dec 28 15 12:46 ]:

[1]@jgm, TBH, my motivation was the code... If this is the same (still,

Since this is a reference implementation, it's important that the code logic correspond clearly to the spec. Simplifying the boolean expressions would obscure the connection, so it's not desirable here...