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

Fix govspeak failures when using IALs on links #75

Merged
merged 2 commits into from May 9, 2016

Conversation

@fofr
Copy link
Contributor

@fofr fofr commented May 9, 2016

Inline attribute lists (IALs) were added to links in Kramdown 1.6.0:
gettalong/kramdown@6279785
http://kramdown.gettalong.org/syntax.html#block-ials
https://github.com/gettalong/kramdown/blob/60795eb95f1588d41e105407add2d3ba6a7fc101/doc/news/release_1_6_0.page

This changed the add_link signature. We monkey patch this in kramdown_with_automatic_external_links to add “rel=external” but we didn’t update that signature when we bumped Kramdown. There were no tests using the IAL feature so the error was not spotted. Live markdown examples do however trigger this feature, which leads to the error:

ArgumentError: wrong number of arguments (5 for 3..4)
  • Include simple IAL external link test
  • Include kramdown’s own IAL link tests
  • Fix bug by updating add_link method signature

Question:

I am not sure why existing markdown in content was triggering this IAL feature.

fofr added 2 commits May 9, 2016
Inline attribute lists (IALs) were added to links in Kramdown 1.6.0:
gettalong/kramdown@6279785
5aae9cc9e2934
http://kramdown.gettalong.org/syntax.html#block-ials
https://github.com/gettalong/kramdown/blob/60795eb95f1588d41e105407add2d
3ba6a7fc101/doc/news/release_1_6_0.page

This changed the `add_link` signature. We monkey patch this in
`kramdown_with_automatic_external_links` to add “rel=external” but we
didn’t update that signature when we bumped Kramdown. There were no
tests using the IAL feature so the error was not spotted. Live markdown
examples do however trigger this feature, which leads to the error:
`ArgumentError: wrong number of arguments (5 for 3..4)`

* Include simple IAL external link test
* Include kramdown’s own IAL link tests
The signature of this method has been changed upstream:
https://github.com/gettalong/kramdown/blob/60795eb95f1588d41e105407add2d
3ba6a7fc101/lib/kramdown/parser/kramdown/link.rb#L38

As part of:
gettalong/kramdown@6279785
5aae9cc9e2934
https://github.com/gettalong/kramdown/blob/60795eb95f1588d41e105407add2d
3ba6a7fc101/doc/news/release_1_6_0.page

Fixes previously committed failing tests.
@boffbowsh boffbowsh merged commit 208f0ea into master May 9, 2016
2 checks passed
2 checks passed
continuous-integration/travis-ci/pr The Travis CI build passed
Details
continuous-integration/travis-ci/push The Travis CI build passed
Details
@boffbowsh boffbowsh deleted the fix-external-link-patch branch May 9, 2016
fofr added a commit that referenced this pull request May 9, 2016
* Fix bug with link parsing introduced in Kramdown 1.6.0 with the
"inline attribute lists" feature  which clashed with our monkey patch

#75
@fofr fofr mentioned this pull request May 9, 2016
fofr added a commit to alphagov/whitehall that referenced this pull request May 9, 2016
* Increase minimum Kramdown version to 1.10.0
* Include support for right aligned columns in table
* Includes fix for previously erroring IAL links:
alphagov/govspeak#75
@fofr fofr mentioned this pull request May 9, 2016
1 of 1 task complete
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Linked issues

Successfully merging this pull request may close these issues.

None yet

2 participants
You can’t perform that action at this time.