-
Notifications
You must be signed in to change notification settings - Fork 56
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
Escape roundtrip #366
Escape roundtrip #366
Conversation
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Thanks @threefjefff for the PR.
Overall I think the PR is going in the right direction. 🎉
You should add a unit test for the preserve-escape
rule and see if it is worth adding a regression test for the issue Dion reported. Perhaps in the markdown-serializer unit test file.
Great work so far! You can see how much code you ended up writing to fix a relatively simple bug - this is why we should be intentional going forward about what is worth fixing and what is not. More code = more maintenance, more cognitive overhead, more bugs potential, etc...
For this specific bug I think we can go ahead and merge once the PR is in a good state with its relevant tests. Thanks for now. 🙂
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Thanks for the bug fix @threefjefff. I ran npm run format
to resolve some of the formatting inconsistencies and get the lint job passing (8c7a3e0). Other than that, the only things I think necessary to get this merged is what Giamir mentioned.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Thanks @threefjefff for adding the tests.
As I was reviewing the tests I realized that original issue reported seems to be fixed exclusively by the state.atBlockStart
expression in the markdown serializer you have added:
const startOfLine: boolean = state.atBlank() || state.atBlockStart || state.closed;
If that's the case we could reduce significantly the amount of code we will need to merge in. We could just add that expression and keep the roundtrip.e2e.test
you created and call it done.
I don't know if I am missing some edge cases; as far as I can tell the tests seem to confirm my theory of the bug being fixed by simply adding state.atBlockStart
. 🤷
Also sorry for realizing this only now.
test/shared/roundtrip.e2e.test.ts
Outdated
[String.raw`\# not a header`], | ||
[String.raw`- \# list item (not header)`], | ||
] as const) { | ||
test(`should make markdown -> richtext -> markdown round trip '${markdown}'`, async () => { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
["lol\n\n***\n\nlmao"], | ||
["`code`"], | ||
//TODO: Codeblock does weird things roundtripping: Adds an extra space | ||
//['```javascript\ncodeblock\n```' ], |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
👍 Good knowledge for a potential follow up ticket (in case it is worth fixing of course).
Great catch! I think this is one of those things where I started fixing things and then was blinded by the implementation. Such is the benefit of a second pair of eyes 🤦♂️
Certainly not for this case. We were discussing these "feel bad" roundtrip errors the other day, and it might be that what's here is a precursor for some of those, but if so I can always refer back to earlier states of this PR. I'll get to stripping this back to the essentials and get back to you. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Great stuff @threefjefff.
I am happy we could sort this issue with less than a line of code in the end. 🎉
I am merging this in.
Closes [#234]
Treat the start of inlining a parent block the same way as the begining of a line (to catch sequences where, for example, we've fought with the editor in rich text mode to allow us to insert a
#
without it turning into a header, thereby serialising to- \#
)PR Checklist
/** ... */
docsbug
/enhancement
and other labels as appropriateEnvironment(s) tested