Skip to content

Conversation

cderv
Copy link
Collaborator

@cderv cderv commented Nov 7, 2024

e.g --max-connections=258 available in R 4.4, or --vanilla

Args should be passed a comma separated list of flags.

This is for expert use and there is not check done on the argument being support by RScript or not.

cderv added 3 commits November 8, 2024 11:46
to pass some flags to Rscript run by Quarto for knitr engine. This can
be useful in specific situation like e.g `--max-connections=258` available in R 4.4, or `--vanilla`

Args should be passed a comma separated list of flags.

This is for expert use and there is not check done on the argument being support by RScript or not.
@cderv cderv force-pushed the knitr/rscript-args branch from fdffa57 to c64046a Compare November 8, 2024 10:46
@cderv cderv requested a review from cscheid November 8, 2024 10:46
Copy link
Collaborator

@cscheid cscheid left a comment

Choose a reason for hiding this comment

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

Setting environment variables like this is risky - environment variables are global options and we can have tests running in parallel. In that case, the environment variables might end up being changed for tests other than this one.

@cderv
Copy link
Collaborator Author

cderv commented Nov 8, 2024

Setting environment variables like this is risky

You mean for the added feature or for the test only ?

For the feature it seems ok to do it like this, as we discussed. Did you have another solution in mind ?

I assume you are talking about the tests.

environment variables are global options and we can have tests running in parallel. In that case, the environment variables might end up being changed for tests other than this one.

I discovered that working on other tests this week where I wanted to use QUARTO_PROFILE and I couldn't make it work. However, my understanding was it was a per test file problem, and that it would not impact other tests in other files.

But from your comment I assume I am wrong. I understand now that we can't really do this at all because deno test can run several test files, and their tests in parallel in same environment ?

I did it initially in #11245 some time ago but discovered this week the possible drawback trying to using QUARTO_PROFILE

Does this mean can't test anything that really on environment variable setting because of asynchronous behavior and that everything works in same environment ?

To understand if I should just remove the test or try to adapt somehow...

@cscheid
Copy link
Collaborator

cscheid commented Nov 8, 2024

I assume you are talking about the tests.

Yes, that's what I was talking about.

@cderv
Copy link
Collaborator Author

cderv commented Nov 8, 2024

So let's just remove those tests then.

I didn't find a great way to tweak environment variable for a single test only.

@cderv
Copy link
Collaborator Author

cderv commented Nov 8, 2024

They could be added back as part of tests that we run with bundled versions. We know we need to run some already.

Or something I did not try... leveraging Quarto project and _environment file ! I'll try that maybe before we merge this.

@cderv cderv force-pushed the knitr/rscript-args branch from 2c2eb74 to 4007a9d Compare November 12, 2024 10:48
@cscheid cscheid merged commit d957b3e into main Nov 14, 2024
47 checks passed
@cscheid cscheid deleted the knitr/rscript-args branch November 14, 2024 15:21
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.

2 participants