Skip to content

Conversation

@adithya-tp
Copy link
Contributor

  1. Make it so that any time programInfo.documentation() is called from any function in src/mlpack/bindings/markdown/, any instances of | are properly escaped (assuming that escaping works with Kramdown). Note that in src/mlpack/bindings/markdown/, any functions just call out to different bindings languages.

This hopefully fixes issue #2270 (escaping does work in kramdown! :)). This should take care of the "|" symbol issue in the markdown for all bindings.

@birm
Copy link
Member

birm commented Mar 11, 2020

This looks good from my end; what testing have you done?

@adithya-tp
Copy link
Contributor Author

This looks good from my end; what testing have you done?

Well, I made the markdown locally, and checked if the problems @rcurtin specified in the issue were resolved with the help of a kramdown editor. Since I saw they were, I went ahead with this pull request.

Copy link
Member

@birm birm left a comment

Choose a reason for hiding this comment

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

Yes, from my end, it looked to resolve the issue perfectly. Thanks for a clever and compact solution!

@adithya-tp
Copy link
Contributor Author

Hey there @birm , sorry, but I just noticed I left an extra "endl" right after printing the description. Just a heads up before I make a commit to fix the same.

Also, thanks for the kind words. Means a lot to me :)

@birm birm self-requested a review March 11, 2020 20:25
Copy link
Member

@birm birm left a comment

Choose a reason for hiding this comment

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

ok, I'll mark it as a requested change until it's removed

@adithya-tp
Copy link
Contributor Author

Thank you, I've made the commit. 👍

@birm birm self-requested a review March 11, 2020 20:44
Copy link

@jeffin-ntx jeffin-ntx left a comment

Choose a reason for hiding this comment

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

🚀

@favre49
Copy link
Member

favre49 commented Mar 13, 2020

@rcurtin mlpack bot broken again? :)

Copy link
Member

@favre49 favre49 left a comment

Choose a reason for hiding this comment

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

Looks fine to me, providing second approval

@favre49 favre49 merged commit 4057806 into mlpack:master Mar 13, 2020
@adithya-tp adithya-tp deleted the markdown-issue-fix branch March 15, 2020 05:39
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Projects

None yet

Development

Successfully merging this pull request may close these issues.

4 participants