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

Provide text escaping and replacement hooks for context-dependent escaping #339

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

Conversation

martincizek
Copy link
Collaborator

Provide text escaping and replacement hooks to allow implementing comprehensive, context-dependent escaping. See #324 as an example, where it is necessary to avoid data corruption.

This PR supersedes #304.

Copy link
Collaborator

@domchristie domchristie left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This looks interesting. I like the idea of text nodes being replaceable (although should they pre or post escaping?)
Anyway, I'm a bit confused as to why we need both an escapes option and a textReplacement option. What would the code look like to customise escaping with these new options?

@martincizek martincizek marked this pull request as draft December 4, 2020 09:00
@martincizek
Copy link
Collaborator Author

martincizek commented Dec 4, 2020

This looks interesting. I like the idea of text nodes being replaceable (although should they pre or post escaping?)
Anyway, I'm a bit confused as to why we need both an escapes option and a textReplacement option. What would the code look like to customise escaping with these new options?

Hi @domchristie, this is related to our e-mail conversation and I should perhaps have it posted here:

One more thing to add about escaping, realized it when walking through the PRs and the README...
I believe we should make TurndownService#escape deprecated in favour of #339.

#339 is now backward-compatible for people who overrode #escape, as the default textReplacement just calls it.
But no other internals are exposed as the main class methods and textReplacement fits well among the other *replacements in advanced options.
I believe it is also a good move towards introducing some pluggable escaping system, regardless of its implementation. It will even allow us to move the escaping system to another project without a fixed dependency.

I will update this PR as follows:

  • the code currently using the escapes setting will be considered the default textReplacement implementation.
  • TurndownService#escape() will be preserved as an extension point just for backward compatibility. It will be marked deprecated and the README will be updated accordingly.
  • the meaning of the escapes option will be better documented. I.e. if the users set it, it will be used by the default textReplacement implementation. If the users provide custom textReplacement implementation, it's up to them how they use the escapes option.

Now it is probably more clear what I meant by eventual externalising of the escaping builder. The builder (or its outcome) would plug into the textReplacement hook and the future escape rules would plug into the builder, should the user use the builder. :)

I consider this the best ratio between backward-compatibility and flexibility in subsequent design decisions. Does this make sense?

@martincizek
Copy link
Collaborator Author

martincizek commented Dec 4, 2020

P.S. Updated the comment to express the intention rather than the code dependencies (the backward compatibility would not work if it were implemented exactly how it was written before :)).

And regarding:

although should they pre or post escaping?

Actually the escaping is the standard thing to do within text node processing. In general, escaping details also depends on the node (i.e. context) and options indeed.

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.

None yet

2 participants