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

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

Open
computamike opened this issue Nov 21, 2020 · 9 comments
Open

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

computamike opened this issue Nov 21, 2020 · 9 comments

Comments

@computamike
Copy link
Contributor

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
Copy link
Owner

vhf commented Nov 21, 2020

Unfortunately it's an issue in Kramdown, see gettalong/kramdown#431 / jekyll/jekyll#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
Copy link
Contributor Author

Unfortunately it's an issue in Kramdown, see gettalong/kramdown#431 / jekyll/jekyll#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
Copy link
Contributor Author

computamike commented Nov 23, 2020

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
Copy link
Contributor Author

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
Copy link
Owner

vhf commented Nov 23, 2020

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
Copy link
Contributor Author

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
Copy link
Contributor Author

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
Copy link
Owner

vhf commented Nov 24, 2020

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
Copy link
Contributor Author

computamike commented Nov 30, 2020

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

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

No branches or pull requests

2 participants