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

Table start_line off by one? #230

Closed
SamWilsn opened this issue Jul 10, 2022 · 6 comments · Fixed by #231
Closed

Table start_line off by one? #230

SamWilsn opened this issue Jul 10, 2022 · 6 comments · Fixed by #231
Assignees

Comments

@SamWilsn
Copy link

I was poking around in comrak (thanks for building this btw!) to track start_line for inline nodes, and ran into some weirdness with tables. I wanted to confirm what the expected behaviour is.

My test case:

| This | is | a | header |
| ---- | -- | - | ------ |
| row  | 1  | b | c      |

This parses into a Table { start_line: 2, .. }. Should the start_line be 1, or is 2 indeed correct?

Thanks!

@kivikakk
Copy link
Owner

Ooh, thanks for the report. I think it should be 1! I'll fix that up.

@kivikakk kivikakk self-assigned this Jul 13, 2022
kivikakk added a commit that referenced this issue Jul 13, 2022
This way matches cmark-gfm, pointing to the header row, which makes a
lot more sense than pointing to the line of the marker row.

Fixes #230.
@SamWilsn
Copy link
Author

Oh, wow, thanks for fixing it so quickly! Really appreciate it :3

I hate to be a pedant, but how do you feel about the top row's TableRow and TableCell nodes having a start_line of 2 still?

@kivikakk
Copy link
Owner

Oh my lord, I really just didn't even check, did I? I'll compare the cmark-gfm behaviour there too, one sec. :)

@SamWilsn
Copy link
Author

SamWilsn commented Jul 13, 2022

If it helps, here's the test case I'm using:

On second thought, this won't really help without my other changes 🤣
    #[test]
    fn inlines_inside_table() {
        let arena = Arena::new();
        let input = r#"| This | is | a | header |
| ---- | -- | - | ------ |
| row  | 1  | b | c      |
"#;

        let config = ComrakOptions {
            extension: ComrakExtensionOptions {
                table: true,
                ..Default::default()
            },
            ..Default::default()
        };
        let lines = parse_document(&arena, input, &config);

        let lines: Vec<_> = lines
            .descendants()
            .filter(|n| matches!(n.data.borrow().value, NodeValue::Text(_)))
            .map(|n| n.data.borrow().start_line)
            .collect();

        assert_eq!(lines, [1, 1, 1, 1, 3, 3, 3, 3]);
    }

@kivikakk
Copy link
Owner

Thanks for that. I'm going to fix this right and add an XML formatter while I'm there, to make test cases for these kinds of things easier. Will be a little bit.

@kivikakk
Copy link
Owner

The eventual fix will come at #232, but if you feel like opening a PR for your specific case, please feel free.

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 a pull request may close this issue.

2 participants