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.

Sign up for GitHub

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

Jump to bottom

Require markdown >= 1.3 and update usage #745

Merged
merged 6 commits into from
Nov 8, 2022
Merged

Require markdown >= 1.3 and update usage #745

merged 6 commits into from
Nov 8, 2022

Conversation

yihui
Copy link
Member

@yihui yihui commented Oct 21, 2022 •
edited

This function will be removed in a future version of markdown.

I'll let you know when this PR is ready to merge. Thanks!

PR task list:

  • Update NEWS
  • Add tests (if possible)
  • Update documentation with devtools::document()

@yihui
Stop importing markdown::markdownExtensions()
06121b1 
This function will be removed in a future version of **markdown**.
@yihui
The extensions and fragment.only arguments will be removed, too
913699c 
using options = 'fragment_only` will be equivalent to `fragment.only = TRUE`
@yihui
Update learnr-package.R
cf0b9aa 
[ci skip]
@yihui yihui mentioned this pull request Oct 21, 2022
Merged
9 tasks
yihui added a commit to rstudio/markdown that referenced this pull request Oct 21, 2022
@yihui
add markdownExtensions() back for now: rstudio/learnr#745
519aa57
yihui added a commit to rstudio/markdown that referenced this pull request Oct 27, 2022
@yihui
add markdownExtensions() back for now: rstudio/learnr#745
c1c1ca1
@gadenbuie
Copy link
Member

Thanks @yihui, is this ready for review?

@yihui
no need to import in namespace; just use markdown::markdownToHTML()
870d8fb
@yihui
require markdown >= 1.3, and call mark_html(options = '-standalone') …
64bc0a6 
…instead of using the deprecated option `fragment_only` (the `mathjax` option is enabled by default, so no need to specify it)
@yihui
add news
f83e864
@yihui yihui marked this pull request as ready for review November 1, 2022 16:20
@yihui
Copy link
Member Author

yihui commented Nov 1, 2022

I just made a new release of the markdown package to CRAN the other day. This PR is ready to merge now (after it passes the checks). Thanks!

@gadenbuie
Copy link
Member

@yihui First, I really appreciate the complete PR and for making the transition easy! That said, I'm looking at learnr's single use of markdown::mark_html() and wondering if it would make more sense for us to also switch to calling commonmark directly. Are there advantages to using the markdown package that we'd miss or have to handle if we switched to commonmark?

@yihui
Copy link
Member Author

yihui commented Nov 3, 2022

Good question. The commonmark package is basically a bare minimal version of Markdown, and the markdown package supports a few extra features such as LaTeX math, base64 encoding images, super/subscripts, etc. If you don't need these extra features, you can definitely just call commonmark::markdown_html().

@gadenbuie gadenbuie changed the base branch from main to rc-v0.11.2 November 8, 2022 02:29
gadenbuie
gadenbuie approved these changes Nov 8, 2022
View reviewed changes
Copy link
Member

@gadenbuie gadenbuie left a comment

Choose a reason for hiding this comment

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

Thanks again @yihui! And thank you for clarifying – we'll stick with {markdown} for now.

@gadenbuie gadenbuie changed the title Stop importing markdown::markdownExtensions() Require markdown >= 1.3 and update usage Nov 8, 2022
@gadenbuie gadenbuie merged commit 2720418 into rc-v0.11.2 Nov 8, 2022
@gadenbuie gadenbuie deleted the remove-markdownExtensions branch November 8, 2022 02:32
gadenbuie pushed a commit that referenced this pull request Nov 8, 2022
@yihui @gadenbuie
Require markdown >= 1.3 and update usage (#745)
8713e2a 
* Stop importing markdown::markdownExtensions()

This function will be removed in a future version of **markdown**.

* The extensions and fragment.only arguments will be removed, too

using options = 'fragment_only` will be equivalent to `fragment.only = TRUE`

* Update learnr-package.R

[ci skip]

* no need to import in namespace; just use `markdown::markdownToHTML()`

* require markdown >= 1.3, and call mark_html(options = '-standalone') instead of using the deprecated option `fragment_only` (the `mathjax` option is enabled by default, so no need to specify it)

* add news
yihui added a commit to rstudio/markdown that referenced this pull request Nov 9, 2022
@yihui
remove warn2(); hopefully rstudio/learnr#745 will be merged soon
c8c46bb
yihui added a commit to rstudio/markdown that referenced this pull request Nov 15, 2022
@yihui
remove markdownExtensions() after rstudio/learnr#745
6ef4431
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@gadenbuie gadenbuie gadenbuie approved these changes

Assignees
No one assigned
Labels
None yet
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.

None yet
2 participants
@yihui @gadenbuie