vhf / free-programming-books-lint

A rudimentary Markdown linter based on remark-lint for the free-programming-books project.
32 stars 12 forks source link

Bug / Idea.... Pipe breaks links #12

Open computamike opened 3 years ago

computamike commented 3 years ago

Links in the form : [ xxx | yyy](zzz) break when rendered using Jekyll / Github Pages.

I found this issue #5176 when looking through the screencasts page : C#

I submitted a PR to fix this issue in all the files I found it in and used the following regular expression to identify it :

^(.*)(\[)(.*)(\|)(.*)(\])

This might need some work - but it seems that the linter might be a good place to highlight if this issue were to occur again.

If I get a chance I'll have a look at implementing this - but I thought it would be a good idea to document this issue here incase someone on the team has more opportunity to fix this than I do.

vhf commented 3 years ago

Unfortunately it's an issue in Kramdown, see https://github.com/gettalong/kramdown/issues/431 / https://github.com/jekyll/jekyll/issues/2818 / https://stackoverflow.com/questions/23751917/how-do-you-disable-tables-in-kramdown and it seems to be a wontfix.

Creating a remark-lint plugin for this would make sense IMO.

computamike commented 3 years ago

Unfortunately it's an issue in Kramdown, see https://github.com/gettalong/kramdown/issues/431 / https://github.com/jekyll/jekyll/issues/2818 / https://stackoverflow.com/questions/23751917/how-do-you-disable-tables-in-kramdown and it seems to be a wontfix.

Creating a remark-lint plugin for this would make sense IMO.

I've started putting together a plugin... It's been a while since I've done node - just figuring out where it should go... Should also check if there are other things that could cause rendering issues...

computamike commented 3 years ago

Hi @vhf - I've put together a plugin based on taking apart the other plugins that are available, and pretty much copying what was there. I have uploaded it into a repo - not sure how to proceed

https://github.com/computamike/remark-link-escape

I could upload it as an NPM package, and then it would just be a case of referencing that from within the free-programming-books-lint?

computamike commented 3 years ago

I've created an NPM package - was able to upload from Command Line - but was hoping to publish from GitHub Action - getting an error so will need to look into it.

https://www.npmjs.com/package/remark-link-escape

vhf commented 3 years ago

Good work! I think the naming of this package is a bit too broad, making it more explicit (and prefixing it with remark-lint-) would be much better for the remark-lint ecosystem. Fortunately npm allows unpublishing packages (releasing package names) up to 72h after initial publication: https://www.npmjs.com/policies/unpublish

Inspired by how other plugins are named here: https://github.com/remarkjs/remark-lint#list-of-external-rules, something like remark-lint-no-pipes-in-links?

After that I'll help you fix your plugin, you don't need to count lines and run regexs over the whole content since the plugin gets an AST. Instead, we should visit the link nodes and check the link title part only. Here's a similar plugin visiting link nodes: https://github.com/vhf/remark-lint-no-url-trailing-slash/blob/master/lib/resource-url.js

computamike commented 3 years ago

Thanks, @vhf - these are great points. I've unpublished the package from npm - great point about the AST - I'll have a go at your suggestions.

computamike commented 3 years ago

Hi @vhf, I've gone ahead and made the changes you suggested.

I'm a bit dubious about the visit within a visit that I have written.

https://github.com/computamike/remark-lint-no-pipes-in-links/blob/46aadbcf4f54de9b4838a7403db4bab259f4cd8a/index.js#L43

I'm not sure if there's a better way of accomplishing this?

I also raise a message per pipe that exists - not sure if I should only raise a single message for a link, or multiple messages for each pipe within a link? - Most instances that were fixed were single instances within a link.

vhf commented 3 years ago

Your visit within a visit is fine: visiting every text node in every link and nothing else. 👍 That way it will detect pipes in any child, eg. [*This is* a **title with a | in**](http://example.com)

I also raise a message per pipe that exists - not sure if I should only raise a single message for a link, or multiple messages for each pipe within a link? - Most instances that were fixed were single instances within a link.

That's up to you, both options make sense in my opinion.

computamike commented 3 years ago

hi @vhf - sorry i've been a bit slow. I have build a v0.1 package, and published it to GitHub.

https://www.npmjs.com/package/remark-lint-no-pipes-in-links