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

Fix over-zealous backslash replacement #378

Open
wants to merge 1 commit into
base: master
Choose a base branch
from
Open

Fix over-zealous backslash replacement #378

wants to merge 1 commit into from

Conversation

jcheatham
Copy link
Contributor

Hi,

Two things, the first is to fix the current travis breakage. I wasn't sure why it started with the last build, but it seems like it was broken since the original commit, so maybe some library or gem update? Just looks like the test just needed a \n though.

Second is for #214 (specifically), didn't fix the original emphasis problem in that issue though, looks like that's in char_emphasis, which I'll try and take a look at that when I can. I couldn't find any instances where the backslash needed to be escaped, but please let me know if there's some case I missed (example test would be awesome! :)

And of course please let me know if this needs something else/more to get merged.

Cheers!

@jcheatham
Copy link
Contributor Author

@robin850 (just in case you didn't get the github ping :)

@robin850
Copy link
Collaborator

Hi @jcheatham,

I wasn't sure why it started with the last build, but it seems like it was broken since the original commit, so maybe some library or gem update?

Yep I think it's a Nokogiri update or something like this as the html_equal helper relies on it.

As for the second change, this is pretty strange as the conformance suite doesn't pass on my computer but it does on Travis.

Auto links ... OK
Backslash escapes ... FAILED

2c2
< <p>Backslash: \\</p>
---
> <p>Backslash: \</p>

Blockquotes with code blocks ... OK

I need to investigate a bit before merging this unfortunately. I will also give some feedback a bit later.

Thanks a ton anyway! ❤️

@jcheatham
Copy link
Contributor Author

No problem, @robin850. Funnily enough this also failed for me at first, but then worked after I got frustrated and did a completely clean build...

The problem is primarily the inconsistency of passing along a single backslash when encountering an unknown escape sequence (e.g. the \p in the test). We could strip the backslash from an unknown sequence or drop the entire sequence altogether... but this seemed like more likely to lead to user confusion, while I couldn't find a strong case for requiring an escape-sequence here.

Happy for any feedback, and merci pour votre attention!

@robin850
Copy link
Collaborator

robin850 commented Jul 7, 2014

Ok, I'm sorry @jcheatham but I will close this one. Actually we can't remove the back-slash from the list of escaped chars and the markdown you've can't generate the output you're expecting. The original Markdown implementation and Redcarpet both return "\server\path" which seems correct.

This is also the behavior of the grep command:

$ cat foo.txt
\\\\server
\\server
\server

$ grep -n "\\\\server" foo.txt
1:\\\\server
2:\\server
3:\server

Moreover, reading the foo.txt file from Ruby and inspecting it returns a string with 16 backslashes for the first line (which seems pretty big but logical).

Feel free to comment if I'm wrong and I will reopen ; thanks for your work on Redcarpet anyway, we really appreciate ! ❤️

@robin850 robin850 closed this Jul 7, 2014
@jcheatham
Copy link
Contributor Author

Hi @robin850,

Oh, I'm definitely familiar with the madness associated with backslash escape sequence expansion + the shell... I just think in this case it's more a matter of expectation & UX. As a user entering text into a field, even though they might be markdown-savvy they typically aren't expecting to have to deal with using 3 or 4 slashes in order to get 2 when a single slash shows up just fine (when it's not part of an escape sequence) as it's inconsistent. They can (hopefully) click "Preview" on whatever form they're using and get feedback and keep adding slashes as necessary, but it's kind of a poor justification to tell them the reason it has to work that way is "well, it's because a bunch of other tools also exhibit this behavior so we'll just perpetuate it". If the escape sequence is necessary for some other functionality I'm totally oblivious to I'd love to hear it of course, otherwise I think we might just have to fork...

hmmm... or what about if this was done through a run-time or compile-time flag to enable the behavior on demand?

Thanks for the consideration, and I hope you'll give this issue just a bit more from a poor confused user's perspective :)

@robin850
Copy link
Collaborator

@jcheatham : At least your points are fair. I will give it some more thoughts ; thanks a lot for your input! :-)

@robin850 robin850 reopened this Jul 12, 2014
@Stargator
Copy link

@jcheatham Can you rebase or resolve the merge conflict?

@jcheatham
Copy link
Contributor Author

holy necromancy @Stargator , I'll dust off whatever recollection I have and give it my best

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.

3 participants