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 Nokogiri Node.new deprecation warnings #296

Merged
merged 2 commits into from
Mar 25, 2022

Conversation

timja
Copy link
Contributor

@timja timja commented Mar 16, 2022

Fixes #295

What’s changed

Deprecation warnings from Nokogiri addressed when rendering markdown tables

Identifying a user need

Not spamming my logs pls and thank you

cc @lfdebrux

Tim Jacomb added 2 commits March 16, 2022 11:07
Reproduces the deprecation warning in the example app
@@ -69,7 +69,7 @@ def table_row(body)
first_child.content = leading_text.sub(/# */, "")
end

tr = Nokogiri::XML::Node.new "tr", fragment
tr = Nokogiri::XML::Node.new "tr", fragment.document
Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

this change can be tested by viewing:
http://localhost:4567/code.html

If you checkout just the first commit you will see the deprecation warning, if you then apply the second commit you will see it fixed

Comment on lines +98 to +100
pre = Nokogiri::XML::Node.new "pre", fragment.document
pre["tabindex"] = "0"
code = Nokogiri::XML::Node.new "code", fragment
code = Nokogiri::XML::Node.new "code", fragment.document
Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Wasn't sure how to test these, it appears to be a fallback for when you don't have a source code highlighter, but I think it should be okay given the other change worked.

</div>
| httpResult | Message | How to fix |
| - | - | - |
| `400` | `[{`<br>`"error": "BadRequestError",`<br>`"message": "Can't send to this recipient using a team-only API key"`<br>`]}` | Use the correct type of API key |
Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@lfdebrux lfdebrux changed the title Pass a document rather than a fragment to new node Fix Nokogiri deprecation warnings Mar 25, 2022
@lfdebrux lfdebrux changed the title Fix Nokogiri deprecation warnings Fix Nokogiri Node.new deprecation warnings Mar 25, 2022
@lfdebrux lfdebrux merged commit a8f0ed8 into alphagov:master Mar 25, 2022
@lfdebrux lfdebrux mentioned this pull request Mar 25, 2022
@timja timja deleted the fix-deprecation-warning branch March 25, 2022 17:16
This was referenced Mar 31, 2022
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.

Passing a Node as the second parameter to Node.new is deprecated
2 participants