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

Add new debug step to knitr capabilities #7013

Merged
merged 10 commits into from Sep 29, 2023
Merged

Add new debug step to knitr capabilities #7013

merged 10 commits into from Sep 29, 2023

Conversation

cderv
Copy link
Collaborator

@cderv cderv commented Sep 26, 2023

This PR aims to help debug further #6960 by

  • Separating checking R binary is working (using RScript --version) and checking R configuration (knitr and rmarkdown package).
  • This allow to throw "Unable to locate R error" only when R found by Quarto is really not working, and throw other error regarding capabilities otherwise.
  • It also allow to add more debug line for when $QUARTO_LOG_LEVEL=DEBUG is set

@cderv cderv marked this pull request as draft September 26, 2023 14:48
@cderv cderv marked this pull request as ready for review September 26, 2023 16:27
Copy link
Collaborator Author

@cderv cderv left a comment

Choose a reason for hiding this comment

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

Review from call with Carlos

src/command/check/check.ts Outdated Show resolved Hide resolved
src/execute/rmd.ts Show resolved Hide resolved
src/execute/rmd.ts Outdated Show resolved Hide resolved
@cderv
Copy link
Collaborator Author

cderv commented Sep 26, 2023

@jjallaire do you know if some of our other tools / products is parsing some error message or results of quarto check ?

I would like to do the same as checkKnitrInstallation() in printCallRDiagnostics() . However, printCallRDiagnostics() is called if there is an error happending while rendering with knitr engine, so not sure about the impact of changing the info message we print.

Do you think it is safe ?

For now, this PR only modifies checkKnitrInstallation() to be safe as this is needed for #6960 but I plan to update printCallRDiagnostics() if possible later.

thanks

@cderv cderv added needs-discussion Issues that require a team-wide discussion before proceeding further and removed needs-discussion Issues that require a team-wide discussion before proceeding further labels Sep 27, 2023
@jjallaire
Copy link
Collaborator

No one that I know of is parsing that output so I think it is safe.

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

2 participants