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

Anticipate more knitr engines in code blocks #1552

Merged
merged 38 commits into from
Oct 7, 2022
Merged

Anticipate more knitr engines in code blocks #1552

merged 38 commits into from
Oct 7, 2022

Conversation

IndrajeetPatil
Copy link
Collaborator

@IndrajeetPatil IndrajeetPatil commented Sep 24, 2022

Closes #796

Related to #797

Plus styling changes.

R/extract.R Outdated Show resolved Hide resolved
@IndrajeetPatil IndrajeetPatil changed the title Anticipate raw HTML in extended knitr engines code blocks Anticipate more knitr engines code blocks Sep 24, 2022
@IndrajeetPatil IndrajeetPatil changed the title Anticipate more knitr engines code blocks Anticipate more knitr engines in code blocks Sep 24, 2022
NEWS.md Outdated Show resolved Hide resolved
R/extract.R Outdated Show resolved Hide resolved
.lintr Show resolved Hide resolved
@IndrajeetPatil IndrajeetPatil removed the request for review from AshesITR September 26, 2022 17:31
@AshesITR
Copy link
Collaborator

AshesITR commented Oct 1, 2022

Maybe we should file an issue with knitr as well?

To ask for guidance?

@MichaelChirico
Copy link
Collaborator

To ask for guidance?

basically, yes. do they recommend package authors register engines in .onLoad? how else might we detect engines in our use case?

@MichaelChirico
Copy link
Collaborator

Filed: rstudio/tufte#117

And: yihui/knitr#2181

@IndrajeetPatil
Copy link
Collaborator Author

Thanks for filing the issues, @MichaelChirico!

@MichaelChirico
Copy link
Collaborator

rstudio/tufte#117 now closed: rstudio/tufte@a097026

What remains to be done on this PR?

@IndrajeetPatil
Copy link
Collaborator Author

IndrajeetPatil commented Oct 3, 2022

What remains to be done on this PR?

I am no longer hardcoding "marginfigure" engine and have updated tests to rely on the devel-{tufte}.

I don't think there is anything else that needs to be done here.

@MichaelChirico
Copy link
Collaborator

Yihui notes in yihui/knitr#2181 there's no strong recommendation for engines to be registered .onLoad(); I still think it best to assume that's done, and tell users to encourage package authors to do so otherwise if they want lintr to work properly.

If there's demand, we could also consider, e.g. a .lintr setting to control custom engines.

NEWS.md Outdated Show resolved Hide resolved
@@ -44,6 +44,25 @@
* `boolean_arithmetic_linter()` for identifying places where logical aggregations are more appropriate, e.g.
`length(which(x == y)) == 0` is the same as `!any(x == y)` or even `all(x != y)` (@MichaelChirico)

## Notes

* `lint()` continues to support Rmarkdown documents. For users of custom .Rmd engines, e.g.
Copy link
Collaborator

Choose a reason for hiding this comment

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

@IndrajeetPatil PTAL. I moved it from "bug fixes" because there's no longer any code change by lintr. I still don't feel like I have a 100% grasp of the situation, so feel free to refine the item further.

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

Thanks, Michael. This looks good to me.

I think we can revise this further as our understanding of the issue evolves while working on #1617.

NEWS.md Outdated Show resolved Hide resolved
Co-authored-by: Indrajeet Patil <patilindrajeet.science@gmail.com>
@codecov-commenter
Copy link

codecov-commenter commented Oct 7, 2022

Codecov Report

Merging #1552 (70d852c) into main (37f7aeb) will not change coverage.
The diff coverage is n/a.

@@           Coverage Diff           @@
##             main    #1552   +/-   ##
=======================================
  Coverage   98.25%   98.25%           
=======================================
  Files          98       98           
  Lines        4352     4352           
=======================================
  Hits         4276     4276           
  Misses         76       76           
Impacted Files Coverage Δ
R/extract.R 100.00% <ø> (ø)

Help us with your feedback. Take ten seconds to tell us how you rate us. Have a feature suggestion? Share it here.

@IndrajeetPatil IndrajeetPatil merged commit 8bec6df into main Oct 7, 2022
@IndrajeetPatil IndrajeetPatil deleted the 796_tufte branch October 7, 2022 06:28
@MichaelChirico MichaelChirico modified the milestones: 3.0.3, 3.1.0 Mar 20, 2023
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.

Support tufte package arbitrary margin content
4 participants