Labels in ATX-style headings which contain a colon are ignored

[robwalton]

I've tracked down an issue that effects the atom and vscode MPE pandoc preview system to the mume library. When these applications use mume during preview to convert a markdown file before sending its contents to pandoc there is a corner case in which mume breaks some ATX-style header labels.

For example when previewing the one line markdown file in MPE:

# My Heading {#sec:mylabel}

mume ignores the label and uses the heading name to create an implicit one:

<p data-line="0" class="sync-line" style="margin:0;"></p>

# My Heading {#my-heading }

This is not a problem when using setext-style headers (that is with underlines). This causes the popular pandoc-crossref filter, which requires section labels to begin with 'sec:', to break.

Sorry for the ticket spam! I created shd101wyy/vscode-markdown-preview-enhanced#1657 with the symptoms and then shd101wyy/vscode-markdown-preview-enhanced#83 once I got closer to the cause.

[robwalton]

I've fired it up in the vscode debugger and discovered that removing the ':' from this line in attributes/parse.ts: fixes the problem. When I get a chance to get the tests running (not a javascript guy!) I'll make a pull request.

[shd101wyy]

@robwalton Awesome. Thanks a lot.

[robwalton]

Since you are here @shd101wyy. I've got mume checked out and built in vscode (I see 'Starting compilation in watch mode...'). Any hints on how to run the tests? (I'm sure obvious to typescript/javascript people!)

[kachkaev]

Thanks @robwalton! It'd be great if you could add a failing test first, then fix the implementation and finally make sure that the new test passes. This will help us avoid the same issue again in future.

Test cases for attributes are in test/lib/attributes.test.ts.

[robwalton]

Thanks @kachkaev I found those. Any hints on how to run them. Sorry, I don't know about typescript or javascript things.

[kachkaev]

You can start tests by running npm run test in a new Terminal tab. You can also try npm run test --watch if you want to run the tests continiously. See jest docs for more options https://facebook.github.io/jest/docs/en/cli.html

I'm happy to help if you have further issues.

[robwalton]

Awesome, thanks. I got the tests running. I'll give it a go later today.

[robwalton]

I've added some failing tests to reflect the issue here and the fix suggested earlier fixes these. However it breaks three existing tests. I'm quite happy to find a way not to break these. However, before I going adding complexity, I wanted to make sure that the failing tests are important.

They are the first two in the suite and the only to use cmd:true to specify a key-value pair rather thancmd=true. I'm pretty sure this is not required by pandoc and its definitely not part of the standard for headers described in the pandoc manual, but maybe there is another use I'm not aware of for this functionality?

Two here:

  {
    // classic behavior
    attributes: { cmd: true },
    raw: [
      "cmd=true",
      "{cmd=true}",
      "  {  cmd=true  }  ",
      "{cmd:true}",       // FAILING
      "cmd:true",        // FAILING
    ],
    stringified: "cmd=true",
  },

And one here

  {
    attributes: { cmd: true, hello: "world" },
    raw: ["cmd=true hello=world", "cmd:true hello:world"],  // SECOND FAILING
    stringified: 'cmd=true hello="world"',
  },

Do we need to support these tests? (If in doubt I will.)

[kachkaev]

I added : as a synonym of = in https://github.com/shd101wyy/mume/commit/4898f1f1b077a725b0fa26f3830a06db51448357 after noticing its use in TOC (see first screenshot).

I'm happy to remove : as a synonym to = but I'll let @shd101wyy decide.

[shd101wyy]

@kachkaev @robwalton Sure we could remove it. Thank you ;)

[kachkaev]

@robwalton feel free to roll back https://github.com/shd101wyy/mume/commit/4898f1f1b077a725b0fa26f3830a06db51448357 and add a test for you case to make sure we don't break it in future. It'd be great to remove any mentioning of : as = from the docs too!

[robwalton]

I've submitted a pull request. @kachkaev

It'd be great to remove any mentioning of : as = from the docs too!

I had a quick look and not sure where you mean on these?

[kachkaev]

The reason why I added : as a synonym for = in https://github.com/shd101wyy/mume/commit/4898f1f1b077a725b0fa26f3830a06db51448357 was because I saw : in the docs (screenshot 1 on TOC page in particular). Give that colons are no longer supported as a delimiter between the key and the value, it'd be great if this was reflected in the dos. @shd101wyy can you recall in what parts of it you could refer to : as =?

[shd101wyy]

@kachkaev Yes we will need to update the doc. The toc is defined as a code chunk. You probably could refer to this line. Thank you.

[shd101wyy]

I just updated the documentation for TOC here. Feel free to remove : synonym. Thank you.

[kachkaev]

@shd101wyy that should be done by @robwalton in shd101wyy/markdown-preview-enhanced#48. The PR LGTM – feel free to review it too and merge.

[robwalton]

Hi guys, sorry for the delay. It would have been cleaner for me to include the doc change in my pull request. Still happy to do so, but looks like its all moved forward now.

[shd101wyy]

@robwalton The documentation is in this repository. If you are going to make any changes, please submit a pull request there. Thank you!

[kachkaev]

@shd101wyy it'd be great if you could look at https://github.com/shd101wyy/mume/pull/48 as well as two other PRs from me and cut a new release 🎉

[shd101wyy]

@kachkaev Sure I will do it this weekend and publish a new version. Thank you ;)