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

Treat .Rmarkdown files as .Rmd files #824

Merged
merged 10 commits into from Jul 15, 2021
Merged

Conversation

JohannesNE
Copy link
Contributor

.Rmarkdown files are like .Rmd files, except they render to .md instead of .html (https://bookdown.org/yihui/blogdown/output-format.html)

Unfortunately they are rejected by styler (Error: Can only style .R, .Rmd and .Rnw files.)

@JohannesNE
Copy link
Contributor Author

If you would rather implement this differently, of if I've overlooked something, feel free to just reject at implement it properly.

@codecov-commenter
Copy link

codecov-commenter commented Jul 15, 2021

Codecov Report

Merging #824 (2e64b07) into master (d31ee93) will increase coverage by 0.03%.
The diff coverage is 100.00%.

Impacted file tree graph

@@            Coverage Diff             @@
##           master     #824      +/-   ##
==========================================
+ Coverage   90.46%   90.49%   +0.03%     
==========================================
  Files          47       47              
  Lines        2432     2441       +9     
==========================================
+ Hits         2200     2209       +9     
  Misses        232      232              
Impacted Files Coverage Δ
R/set-assert-args.R 92.68% <100.00%> (ø)
R/ui-styling.R 100.00% <100.00%> (ø)
R/utils-files.R 100.00% <100.00%> (ø)

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update d31ee93...2e64b07. Read the comment docs.

@github-actions
Copy link
Contributor

This is how benchmark results would change (along with a 95% confience interval in relative change) if eede456 is merged into master:

  • cache_applying: 0.09s -> 0.09s [-3.12%, +3.97%]
  • cache_recording: 0.96s -> 0.96s [-0.83%, +2.66%]
  • without_cache: 2.4s -> 2.38s [-1.09%, +0.05%]

Further explanation regarding interpretation and methodology can be found in the documentation.

@lorenzwalthert
Copy link
Collaborator

Thanks @JohannesNE. Closes: #480. Before a merge, we should:

  • add tests: tests/testthat/test-public-api.R. Look for Rmd and add one test for every API function (style_file(), style_dir(), style_pkg()). Just look for 'Rmd' in that file (starting around line 134).
  • Add a news bullet: Files with .Rmarkdown extensions are now recognised as an R markdown files in style_file() and friends (Treat .Rmarkdown files as .Rmd files #824).
  • In the documentation, state explicitly that the new extension is treated as R markdown, i.e. adapt the documentation of the parameter filetype in prettify_pkg().

Do you want to give this a try?

@JohannesNE
Copy link
Contributor Author

My experience is limited, but I have given it a try.

NEWS.md Outdated Show resolved Hide resolved
@lorenzwalthert
Copy link
Collaborator

@JohannesNE looks great overall, thanks a lot. Let's keep our fingers crossed for the tests to pass 🙏.

If I use locally produced ref files, CI fails on all machines, so these must be peculiarities of this machine
@JohannesNE
Copy link
Contributor Author

Sorry. I actually meant to make a comment that i was unsure about committing the updated reference files, but forgot about it.

@lorenzwalthert
Copy link
Collaborator

lorenzwalthert commented Jul 15, 2021

There is something strange going on with some unrelated reference files I think. When I locally run the tests, I get modified files (so tests fail), but on CI/CD, the same reference files pass (I know from previous experience). I reverted to the old ref files in 2e64b07, let's hope we can make tests pass. Maybe I'll open a separate issue. The problem persists for some time already.

@lorenzwalthert
Copy link
Collaborator

Are you working on macOS as well?

@JohannesNE
Copy link
Contributor Author

I'm on Ubuntu 21.04. I am afraid won't be of much help with that problem. I usually "fix" encoding problems by trial-and-error 🤷‍♂️

@github-actions
Copy link
Contributor

This is how benchmark results would change (along with a 95% confience interval in relative change) if 49c52ed is merged into master:

  • cache_applying: 0.1s -> 0.1s [-3.61%, +3.75%]
  • cache_recording: 1.06s -> 1.07s [-0.07%, +0.44%]
  • without_cache: 2.69s -> 2.69s [-0.13%, +0.4%]

Further explanation regarding interpretation and methodology can be found in the documentation.

@lorenzwalthert
Copy link
Collaborator

I'm on Ubuntu 21.04. I am afraid won't be of much help with that problem. I usually "fix" encoding problems by trial-and-error 🤷‍♂️

Ok. Seems not a problem in CI/CD, neither on CRAN, so I guess we can leave it for now.

@lorenzwalthert lorenzwalthert merged commit e7777f2 into r-lib:master Jul 15, 2021
@lorenzwalthert
Copy link
Collaborator

Thanks @JohannesNE 🥳

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.

None yet

3 participants