-
-
Notifications
You must be signed in to change notification settings - Fork 149
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
Write a sourcepos test for each NodeValue
variant
#498
Conversation
Some of these are covered at the bottom of the specific test file, such as https://github.com/kivikakk/comrak/blob/main/src/tests/wikilinks.rs#L218-L276, though not everything is covered. So more coverage of this is super helpful. Yeah, there is still more work to do on getting inline sourcepos working consistently. |
How do you feel about the whole |
be9fb71
to
12ebd94
Compare
List // end is 3:0 | ||
| Item // end is 3:0 | ||
| TaskItem // end is 4:0 | ||
| DescriptionItem // end is 4:0 | ||
| DescriptionTerm // end is 3:0 | ||
| DescriptionDetails // end is 4:0 | ||
| HtmlInline // end is 1:31 but should be 3:14 | ||
| LineBreak // start is 1:15 but should be 1:13 | ||
| Code // is 1:8-1:12 but should be 1:7-1:13 | ||
| ThematicBreak // end is 4:0 | ||
| Link // inconsistent between link types | ||
| Math // is 3:2-3:6 but should be 3:1-3:7 |
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.
If y'all can confirm that all of these are bugs, I'll try my hand at fixing them.
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.
Yeah I think those are all incorrect sourcesos 😢
12ebd94
to
38f2403
Compare
38f2403
to
6282d75
Compare
👋 A lot has changed, so I've brought this PR up-to-date! I really like the approach, and I'd be happy to merge it now, rather than wait for all sourcepos to be fixed first (which will probably necessitate another big rebase). Some of the changes in this branch have already landed in slightly different form, so a few commits got omitted; please feel free to restore to your version (or whatever you like), my force push here is mostly a suggestion in case you're a fan! |
Thank you! I haven't had time to work on this in... four months apparently 😅 Go ahead with whatever commits you think are best. |
6282d75
to
2055a9e
Compare
Thanks so much! |
Not sure if these are already covered elsewhere. I wrote one for my own knowledge, and kept going... A few are disabled because I believe the returned sourcepos are incorrect.
Apologies for stacking this on top of #497. Looking at just 38f2403 might be best.