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

Add sourcepos info for inlines #228

Merged
merged 4 commits into from
Nov 5, 2017

Conversation

kivikakk
Copy link
Contributor

@kivikakk kivikakk commented Aug 9, 2017

In github#53 we added sourcepos information for inlines; here's that PR targeting upstream, if you'd like!

/cc @jgm

@kivikakk
Copy link
Contributor Author

kivikakk commented Aug 9, 2017

This produces incorrect values in some cases. Sample:

kahekkass ~/github/cmark master$ echo '**Hello**' | build/src/cmark -t xml --sourcepos
<?xml version="1.0" encoding="UTF-8"?>
<!DOCTYPE document SYSTEM "CommonMark.dtd">
<document sourcepos="1:1-1:9" xmlns="http://commonmark.org/xml/1.0">
  <paragraph sourcepos="1:1-1:9">
    <strong sourcepos="1:1-1:9">
      <text sourcepos="1:3-1:7">Hello</text>
    </strong>
  </paragraph>
</document>
kahekkass ~/github/cmark master$ echo '# **Hello**' | build/src/cmark -t xml --sourcepos
<?xml version="1.0" encoding="UTF-8"?>
<!DOCTYPE document SYSTEM "CommonMark.dtd">
<document sourcepos="1:1-1:11" xmlns="http://commonmark.org/xml/1.0">
  <heading sourcepos="1:1-1:11" level="1">
    <strong sourcepos="1:1-1:9">
      <text sourcepos="1:3-1:7">Hello</text>
    </strong>
  </heading>
</document>

In the second case, the <strong …> should have a different sourcepos to the same element in the first case (because it's two characters over), but it doesn't. Will update this PR soon.

@kivikakk
Copy link
Contributor Author

Fix made; there were two issues:

  1. We weren't adding an offset to the columns of inlines nested inside heading elements.

  2. Newlines in non-text inlines (code, raw HTML) were not being noticed, and we weren't updating column/line numbers accordingly.

@kivikakk
Copy link
Contributor Author

kivikakk commented Nov 2, 2017

Resolved conflicts from latest master to stop this falling too far behind; @jgm, any interest in merging? If not, I'll close and stop keeping it current. 👍

@nwellnhof
Copy link
Contributor

The new field internal_offset in struct cmark_node seems to be used only for headings. I think we could get rid of that field if we added an offset parameter to cmark_parse_inline that contains the current offset from struct cmark_parser.

@kivikakk
Copy link
Contributor Author

kivikakk commented Nov 3, 2017

I've taken a look into this and I'm not sure I see an obvious way to propagate this information correctly without storing it in the node.

@jgm
Copy link
Member

jgm commented Nov 5, 2017

@kivikakk I am interested, sorry for the silence on this, and thanks for updating the PR.

@jgm jgm merged commit b40ecdc into commonmark:master Nov 5, 2017
@kivikakk kivikakk deleted the upstream/inline-sourcepos branch November 5, 2017 23:13
@kivikakk
Copy link
Contributor Author

kivikakk commented Nov 5, 2017

@jgm not at all, thank you!

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.

3 participants