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

Enhance build for rnw files #2013

Merged
merged 9 commits into from
Apr 3, 2020
Merged

Enhance build for rnw files #2013

merged 9 commits into from
Apr 3, 2020

Conversation

jlelong
Copy link
Collaborator

@jlelong jlelong commented Mar 29, 2020

Related to #2005

This PR implements

  • alt+ctrl+v to view the pdf file
  • Accept projects with several .rnw files
  • alt+ctrl+b to trigger the build
  • Choose an appropriate default recipe for .rnw files.
  • The following extensions are associated to the Rsweave language Id : .[rRsS]nw, .[sR]tex
  • Report LaTeX errors/warnings in the Problems pane

This is how .rnw files can be included.
It is used to discover the TeX children.
…r rsweave languageId- If no recipe is specified, the first recipe whose name contains rnwor rsweave is used.
@jlelong jlelong marked this pull request as ready for review March 30, 2020 21:09
In %DOC% and %DOCFILE%, we now remove any extesion, not only .tex

The new two placeholders %DOC_EXT% and %DOCFILE_EXT% are respectively
the root file full path and the root file name with the extension kept.
@jlelong
Copy link
Collaborator Author

jlelong commented Mar 31, 2020

This PR provides a quite satisfying support for .rnw files. Yet, there are two difficulties related to the same technical problem

  • The logs showed in the Problems pane refer to the .tex file not the .rnw one
  • Synctex links the .tex and .pdf files. We would rather use the .rnw file instead of the .tex one.

Fixing these points requires an important effort because we have to introduce the mapping between the .tex and .rnw files defined by the generated -concordance.tex file generated by knit when setting knitr::opts_knit$set(concordance = TRUE);. Once the mapping is available, it must be introduced both in the synctex and log parsing mechanisms. This will undoubtedly introduce a lot of code change. Is it worth it?

@RaghuNaik @maikol-solis @ChristopheRoyer21 @armarti You all showed some interest in editing .rnw using LaTeX-Workshop. How useful would the concordance mechanism be?

@jlelong
Copy link
Collaborator Author

jlelong commented Mar 31, 2020

This PR fiddles with the placeholders and the build mechanism. It requires some heavy testing before merging.

@tamuratak Could you have a look at it when you have time? Sometimes I am wondering if we (I ?) do not go too far beyond the scope of the extension to offer full .rnw support.

@tamuratak
Copy link
Contributor

VS Code is supporting Jupyter Notebook. See microsoft/vscode#93265. I guess that users who want to embed R code and graphics to their documents will start using that directly. I am not sure supporting rnw is promising.

I never use R. So this is just a guess.

I have a question. I am not sure why we cannot detect languageId from the suffix of rootFile.

const findMethods = [
() => this.findRootFromMagic(),
() => this.findRootFromActive(),
() => this.findRootFromCurrentRoot(),
() => this.findRootInWorkspace()
]
for (const method of findMethods) {
const res = await method()
const rootFile = res[0]
const languageId = res[1]
if (rootFile === undefined) {
continue
}
if (this.rootFile !== rootFile) {
this.extension.logger.addLogMessage(`Root file changed from: ${this.rootFile} to ${rootFile}. Find all dependencies.`)
this.rootFile = rootFile
this.rootFileLanguageId = languageId

@jlelong
Copy link
Collaborator Author

jlelong commented Apr 1, 2020

I do not use R that much myself but a lot of colleagues of mine do and they .rnw quite a lot to generate several PDF from a single .tex file with different data sets for instance. It seems to be quite popular. I think it is a good point for the extension to support .rnw file. The real question is how elaborate the support should be (concordance stuff). For the moment, I do not plan to go beyond this PR.

I am not sure why we cannot detect languageId from the suffix of rootFile.

This is a really good question and that was actually my first thought. Then, I thought that it would be more robust to read the document.languageId property when available. Looking back on this, I think you are right as the document.languageId property is only available in findRootFromActive. I will change this.

@tamuratak
Copy link
Contributor

I have no reason to be against this PR if you can see real use cases.

@ChristopheRoyer21
Copy link

Same here, it looks fine to me.

@jlelong jlelong merged commit 750e700 into master Apr 3, 2020
@jlelong jlelong deleted the 2009-build-rsweave branch April 3, 2020 08:46
@github-actions github-actions bot locked as resolved and limited conversation to collaborators Apr 4, 2023
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants