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

Improve handling of newline in inline code block #210

Merged
3 commits merged into from May 3, 2022

Conversation

mikeando
Copy link
Contributor

@mikeando mikeando commented May 2, 2022

This case `\n` was getting parsed as <code></code> rather than <code> </code> due to the \n getting flagged as a non-space character. Change is pretty simple

There's actually an unrelated commit with this that changes error reporting to use #[track_callers] that makes test failures point to the right line (rather than inside the compare_strings helper function)

I can break this into two PRs if you want -- but they seemed trivial enough to review together.

Before this test failures were assigned to the location inside
`compare_strs`. 


With the change the error is assigned to the actual
call-site of the `html(A,B)` check.
src/strings.rs Outdated Show resolved Hide resolved
@mikeando
Copy link
Contributor Author

mikeando commented May 2, 2022

Ugh, there is something trickier going on than I'd thought.
The same node ends up getting processed for inlines more than once.
It will be a day or so before I have time to dig through what is going on...
Changing this PR to WIP until then.

@mikeando mikeando marked this pull request as draft May 2, 2022 06:28
@mikeando mikeando force-pushed the inline_code_newline_handling branch from 1ef778f to c3c6707 Compare May 2, 2022 13:07
@mikeando
Copy link
Contributor Author

mikeando commented May 2, 2022

The trouble I was having was actually with the round-trip tests, not where I thought it was.
They're fixed too now I think. And all the spec tests pass on my machine.

The cause ofmy confusion was that I didn't realise the html function was doing more than just
checking the output matched the provided HTML (and actually does an additional round-trip check).
I then added a bunch of debug prints that sent me down the wrong path altogether. Spent a long time
trying to understand why my Code node was getting processed twice... with different content.

Anyway, sorry about the delay on this.

@mikeando mikeando marked this pull request as ready for review May 2, 2022 13:15
@mikeando mikeando changed the title Improve handling of newline in inline code bloack Improve handling of newline in inline code block May 2, 2022
@ghost ghost force-pushed the inline_code_newline_handling branch from c52ab55 to f6d6117 Compare May 2, 2022 23:41
@ghost
Copy link

ghost commented May 3, 2022

ta, this looks superb! I ran cargo fmt to clear up the last check and pushed. It looks like this change mighta crept in between the last two CM spec versions — at some point we should actually make sure we're compliant with 0.30.

@ghost ghost merged commit 83702f9 into kivikakk:main May 3, 2022
@ghost
Copy link

ghost commented May 3, 2022

if you want a release with these changes, give us a shout, otherwise we'll just wait until there's more accumulated.

@mikeando
Copy link
Contributor Author

mikeando commented May 3, 2022

No need for a new release. I'm happy to point my cargo at the commit for now.

@mikeando mikeando deleted the inline_code_newline_handling branch May 3, 2022 01:44
This pull request was closed.
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

1 participant