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

allow passing arguments down to source() from loadConfig() #98

Open
wants to merge 1 commit into
base: master
Choose a base branch
from

Conversation

mshvartsman
Copy link

When using BatchJobs in moderately sized codebases, it can be helpful to use relative paths for both .tmpl and config files. loadConfig() uses sys.source() (via sourceConfFile()), which by default sources the config file relative to the current path, not the path of the config file itself. There are a number of ways to address this -- I think I picked the least disruptive (but maybe most awkward), which is passing arguments from loadConfig() down to sys.source(), and passing chdir=T to evaluate paths in the config file relative to its own directory.

…onfig to use relative paths for its .tmpl files, which is useful in being able to organize (bigger) codebases that rely on BatchJobs.
@mshvartsman
Copy link
Author

I should add that personally, think a better solution is to default to chdir=T on line 18 of conf.R. I see three use cases:

  1. User evaluates loadConfig() with the working directory set to the config file. Then relative template paths within the config work fine, but they would also work fine with chdir=T.
  2. User evaluates loadConfig() from another working directory, and the template path is relative to the config directory. Then loadConfig() fails to find the template without chdir=T.
  3. User evaluates loadConfig() from another working directory, but the template path is relative to that other working directory. Then loadConfig() fails to find the template with chdir=T.

The pull request as it is now doesn't change the status quo: 1 and 3 still work, 2 still breaks (but can now be fixed by the user passing chdir). My sense is that 1 and 2 are the more reasonable ones to support, but you know the BatchJobs userbase and use cases better. I am happy to revise this pull request to default to chdir=T if you prefer, with or without the ellipsis. Can also pass chdir explicitly (with default T or F) instead of ellipsis.

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.

1 participant