search

interagent / prmd

JSON Schema tools and doc generation for HTTP APIs
MIT License
2.1k stars 172 forks source link

ensure regexp in tables is rendered correctly #347

Closed hannesfostie closed 3 years ago

hannesfostie commented 3 years ago

In https://github.com/interagent/prmd/pull/345/ a bug was fixed where a regexp containing a pipe character would close the table cell it was in. Because the fix uses <pre> instead of the backticks notation for inline code in markdown, the content of the <pre> element was being parsed as markdown.

It is not unusual for a regexp to contain what looks like a markdown link ([...](...)) so that introduced a new bug.

I believe this PR will fix that bug, while introducing a new test case for them

hannesfostie commented 3 years ago

Leaving a comment here for better visibility as the diff is now outdated.

@geemus wrote:

Why the change from | to |? It looks like 124 is the right HTML entity from brief web searching, so I'm not saying this is wrong, just wanted to confirm. Also, if this is using 124, the test below should also use it. I think being consistent between the two places is important, even if I'm more indifferent about which of the two we choose (presuming they both render in the desired way).

There was no particular reason, I think 124 is just the first one I came across when I googled it as I couldn't remember it at the time. Since you brought it up I made sure to google and check if there is in fact a preferred one, but it seems they're all valid. I went with a third option however: named html entities. I feel like these are easier to read than the codes which you almost certainly have to look up. At least in this version, there's a chance you'll recognize it.

I made a third change as well: adding a replacement for asterisks. I thought of that while making this change, and figured it could technically suffer from similar issues as parts of the regex being rendered as a link. Makes me wonder if there's even more that we should be replacing, but I think this is a good start.

What do you think?

geemus commented 3 years ago

Looks good to me. I like the shift to named entities, definitely makes the code/tests easier to read and understand. Hopefully that set at least mostly covers things, though as you suggest maybe there are other things we are forgetting about. Thanks for working through this with me!