Option to preserve special character escapes (for #51) #71

SichangHe · 2024-05-25T16:06:08Z

This is my stab at #51.

This implementation adds a derivative cmark_resume_with_source_range_and_options function that behaves like cmark_resume_with_options but preserves special character escapes, i.e., only escapes if the Markdown source escapes, [`Vec`] ⇒ [`Vec`] and \[`Vec`\] ⇒ \[`Vec`\]. To achieve that, this implementation:

Takes the Markdown source text as an additional argument, and takes an optional "source range" accompanying each event to map back to the source. This is necessary because otherwise we can't know what the escapes were. The source ranges come with pulldown_cmark::OffsetIter; it is optional because we anticipate that the user might manipulate the events, in which case additional events would not has any accompanying ranges.
Adds a field last_event_end_index to State to keep track of the end index of previous events. A gap between the start of the current Event::Text and the end of the previous event indicates there is something in between, which could be a \ or markup like [ before a link or > before a quote. In case of an event being deleted during manipulation, there is an artificial gap, but I bet it would be extremely rare if the source before ends with \ by accident.
In the case of a gap before an Event::Text, this implementation checks if the last character in the gap is \; if not, it performs a hack to prevent the escape. The hack is to set State::is_in_code_block to true. Maybe we should also do back off (check if the \ is escaped, etc.).

Regarding the complex function signature, we could definitely make a trait and implement it for both Iterator<Item = (E, Option<Range<usize>>)> and Iterator<Item = (E, Range<usize>)> so people can simply pass in pulldown_cmark::OffsetIter without the .map(|(e, r)| (e, Some(r))).

And, yes, the names could be improved.

What do you guys think? @Byron, @ytmimi.

<Byron#51>

new `State::last_event_end_index` to track event end source_mapping → source_range

…fferent

Byron · 2024-05-25T19:17:10Z

Wow, this is a big one, thanks so much for tackling this!

All tests pass, additional tests have been added, and the spec now passes 435 of 649 tests, previously it was 435 as well.

Tomorrow I will take a closer look, while hoping that @ytmimi can also join in as I am not too invested into this crate at the moment so I don't necessarily know what's desirable, all I can do is check code.

Byron

Thanks again for tackling this!

I have a few initial comments, as I ran out of time during review, I couldn't get through all the changes just yet. Disposition is merge, I'd just like to work on a few details.

Note to self: don't forget to make this a breaking release due to the added field in state.

Byron · 2024-05-28T05:59:36Z

src/lib.rs

@@ -62,6 +69,8 @@ pub struct State<'a> {
    pub current_shortcut_text: Option<String>,
    /// A list of shortcuts seen so far for later emission
    pub shortcuts: Vec<(String, String, String)>,
+    /// Index of the end of the last event.


Can you clarify in the docstring what this index points into? It should probably contain docstring-links to the function that matters to it, where I believe it would point into source.

src/lib.rs

Byron · 2024-05-28T06:06:34Z

src/source_range.rs

@@ -0,0 +1,112 @@
+use super::*;
+
+/// Serialize a stream of [pulldown-cmark-Events][Event] each with source string into a string-backed buffer.


Suggested change

/// Serialize a stream of [pulldown-cmark-Events][Event] each with source string into a string-backed buffer.

/// Serialize a stream of [pulldown-cmark-Events][Event] while preserving escape characters using `source` for lookup.

Byron · 2024-05-28T06:07:57Z

src/source_range.rs

+///
+/// Different from [`cmark_resume_with_options`], which always escape Markdown special characters like `#` or `[`,
+/// this function preserves special character escapes.
+/// That is, it only escapes a special character if it is escaped in the source.


Can this be clearer? With the sentence preceding it, it feels like this says the opposite - it's supposed to preserve special character escapes?

src/source_range.rs

ytmimi · 2024-05-28T15:25:38Z

I can try to review this PR, though I probably won't be free to take a look at this until later this week

Byron · 2024-05-28T17:46:52Z

Thanks for letting me know - I will wait with merging after your review is in, thanks a lot for your help!

Byron

Thanks a lot for this lovely contribution, it's much appreciated and great work!

I have one more question regarding the added tests: Can it be that these are mostly a duplicate of the existing tests?

If so, would it be possible to adjust the existing fmts (in the old tests) to run the source tests instead (or in addition)? It would definitely be preferable to not introduce code-duplication here.

src/source_range.rs

Byron · 2024-05-29T05:48:41Z

tests/source_range_fmt.rs

+};
+
+fn fmts(s: &str) -> (String, State<'_>) {
+    let mut buf = String::new();


Here fmts_with_options() could be called, right? That would avoid this duplication.

That would assume cmark_with_source_range calls cmark_resume_with_source_range_and_options with the default option. This not is defined in the test module itself, so I would avoid such an assumption.

That's a valid point! Thank you.

tests/source_range_fmt.rs

Byron#71 (review)

SichangHe · 2024-05-30T02:48:06Z

Hi @Byron, I have attempted to address your clean-up suggestions. You might still want to double check the wording for some of the docstrings, etc., and maybe come up with better names for the new public functions…

Byron

Thanks a lot! Looks great now and I am happy we could get rid of most of the duplication.

I will wait for @ytmimi as another set of eyes, but I also ask @SichangHe to ping me for a merge in case that doesn't happen within a week or so as I think whenever the second review happens, changes can always be made in a second PR at least as long the public API isn't affected.

Byron · 2024-05-30T05:29:50Z

tests/source_range_fmt.rs

+};
+
+fn fmts(s: &str) -> (String, State<'_>) {
+    let mut buf = String::new();


That's a valid point! Thank you.

This is marked as breaking change as the field itself is new in `State` and could be breaking for some.

max-heller · 2024-06-10T01:14:19Z

src/source_range.rs

+/// Different from [`cmark_resume_with_options`], which always escape Markdown special characters like `#` or `[`,
+/// this function only escapes a special character if it is escaped in `source`.


Is there any way we could avoid adding escapes in the other cmark variants as well, or does this fundamentally require the source?

@max-heller, this variant "preserves escapes", i.e., adds escapes based on whether the source adds escapes, so it requires the source.

Should've been more explicit, sorry. Obviously we can't preserve escapes without the source, but could we prevent adding them without it? For instance, keep [foo] as [foo] but turn \[foo\] to [foo]? Or would that run in to correctness issues

Byron · 2024-06-10T06:05:29Z

Thanks again for this fantastic contribution!

Here is the new release: https://github.com/Byron/pulldown-cmark-to-cmark/releases/tag/v14.0.0 .

…ce (#95) Using Byron/pulldown-cmark-to-cmark#71, attempts to preserve escape characters (or lack thereof) from the Markdown source, e.g. keeping ``[`Vec`]`` as ``[`Vec`]`` and ``\[`Vec`\]`` as ``\[`Vec`\]``. Closes #91

ytmimi · 2024-06-12T13:29:48Z

Sorry I didn't get a chance to look this one over, but glad to see that it got merged!

SichangHe added 7 commits May 25, 2024 17:08

failing rustdoc test

8c6ccdb

<Byron#51>

extract nested functionalities

93cbb33

placeholder functions not using source range

a234ed5

source_range persist special character escape by peeking source

92ad2c8

new `State::last_event_end_index` to track event end source_mapping → source_range

better explain why cmark_resume_with_source_range_and_options is di…

49f60d5

…fferent

rm test that fail for old implementation

78965c4

restore State derives

ea17e27

test < escape handling

542bc1f

Byron reviewed May 28, 2024

View reviewed changes

Byron added the question Further information is requested label May 28, 2024

Byron reviewed May 29, 2024

View reviewed changes

SichangHe added 2 commits May 30, 2024 10:02

attempt to address @bryon first comments

bebff1e

Byron#71 (review)

dedup test code

f66724b

Byron approved these changes May 30, 2024

View reviewed changes

tests/source_range_fmt.rs

};

fn fmts(s: &str) -> (String, State<'_>) {

let mut buf = String::new();

Copy link

Owner

Byron May 30, 2024

Choose a reason for hiding this comment

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

That's a valid point! Thank you.

fix!: clarify what last_event_end_index indices into and how it's used

6c8297c

This is marked as breaking change as the field itself is new in `State` and could be breaking for some.

Byron force-pushed the no-unnecessary-escape branch from c25bf76 to 6c8297c Compare May 30, 2024 05:41

SichangHe mentioned this pull request May 30, 2024

Call for PR: use Pulldown-CMark to extract match expressions lzanini/mdbook-katex#116

Open

Byron mentioned this pull request Jun 2, 2024

Support For "Math Mode" From pulldown-cmark #72

Closed

max-heller mentioned this pull request Jun 10, 2024

Pre-processing breaks filter syntax max-heller/mdbook-pandoc#91

Closed

max-heller reviewed Jun 10, 2024

View reviewed changes

Byron removed the question Further information is requested label Jun 10, 2024

Byron merged commit 386e663 into Byron:main Jun 10, 2024
1 check passed

max-heller mentioned this pull request Jun 10, 2024

feat: preserve escape characters (or lack thereof) from Markdown source max-heller/mdbook-pandoc#95

Merged

Byron mentioned this pull request Jun 10, 2024

cmark_with_source_range_and_options escapes special characters within code blocks when no range is available #73

Closed

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Option to preserve special character escapes (for #51) #71

Option to preserve special character escapes (for #51) #71

SichangHe commented May 25, 2024

Byron commented May 25, 2024

Byron left a comment

Byron May 28, 2024

Byron May 28, 2024

Byron May 28, 2024

ytmimi commented May 28, 2024

Byron commented May 28, 2024

Byron left a comment

Byron May 29, 2024

SichangHe May 30, 2024 •

edited

Loading

Byron May 30, 2024

SichangHe commented May 30, 2024

Byron left a comment

Byron May 30, 2024

max-heller Jun 10, 2024

SichangHe Jun 10, 2024

max-heller Jun 10, 2024

Byron commented Jun 10, 2024

ytmimi commented Jun 12, 2024

		@@ -0,0 +1,112 @@
		use super::*;

		/// Serialize a stream of [pulldown-cmark-Events][Event] each with source string into a string-backed buffer.

		/// Different from [`cmark_resume_with_options`], which always escape Markdown special characters like `#` or `[`,
		/// this function only escapes a special character if it is escaped in `source`.

Option to preserve special character escapes (for #51) #71

Option to preserve special character escapes (for #51) #71

Conversation

SichangHe commented May 25, 2024

Byron commented May 25, 2024

Byron left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

ytmimi commented May 28, 2024

Byron commented May 28, 2024

Byron left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

SichangHe May 30, 2024 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

SichangHe commented May 30, 2024

Byron left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Byron commented Jun 10, 2024

ytmimi commented Jun 12, 2024

SichangHe May 30, 2024 •

edited

Loading