Fix string escaping in markdown table #49
Conversation
| (-> str | ||
| (str/replace \newline \space) | ||
| (str/replace "|" "\\\\|"))) |
There was a problem hiding this comment.
by changing this function I believe you are passing twice on the full string instead of taking advantage of making a single pass.
Also, it looks like the logic is changed, not sure why we previously replaced \| by \\| instead of | by \\| but this might break something elsewhere.
Edit: I jumped on this PR without reading your description first. So ok that makes sens, I still wonder if that would be possible to do this in a single pass perhaps by using a function instead of a map to be able to sneak a default value en prevent NPE.
There was a problem hiding this comment.
I'm also confused by the logic. Firstly, I think the "\\|" in the previous code was wrong. It doesn't agree with the regex AFAICT.
user=> (re-matches #"(\n|\|)" "|")
["|" "|"]
user=> (re-matches #"(\n|\|)" "\\|")
nilSecondly, yes, the double escaping is strange. The regex matches |, then the map rewrites \| to \\|. It doesn't seem internally consistent. My hunch is that this PR should be changed to escape | to \|.
Regarding perf, I agree it would be nice to do this in one pass. I think str/escape might be a better tool for the job.
The idea behind this function is to escape newlines and pipes in particular places, but it throws a NPE because
str/replaceis used incorrectly. There's a way to fix the replacement map by using vector keys but I wasn't comfortable using it and has already proven error-prone.