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

Specifying --template-variables-path multiple times #18

Closed
dhimmel opened this issue Nov 8, 2017 · 5 comments
Closed

Specifying --template-variables-path multiple times #18

dhimmel opened this issue Nov 8, 2017 · 5 comments

Comments

@dhimmel
Copy link
Member

dhimmel commented Nov 8, 2017

Presently, you can pass manubot a JSON file via the --template-variables-path argument to use for templating. To retain provenance, one preferred way to use this argument is to pass a versioned URL (e.g. with a git commit hash) that points to the JSON file.

However, a single manuscript likely draws from many analyses, which may each have their own JSON outputs. Would it make sense to be able to specify --template-variables-path multiple times to load multiple JSON files for templating?

I think so. My main question is whether each JSON should be namespaced. For example,

--template-variables-path analysis_a=path_a.json
--template-variables-path analysis_b=path_b.json

Namespacing makes it safe for multiple JSON files to contain the same keys. Or should we forgo namespacing (which would be backwards compatible) but risk potential collisions. Also namespaces would add an extra level when specifying a template variable resulting in longer source.

@agitter what do you think?

@agitter
Copy link
Member

agitter commented Nov 8, 2017

Namespacing would be safest, but I'm not sure that it is worth the extra overhead in the template variables. My initial inclination would be to allow multiple JSON files and have later files override keys in earlier files (or vice versa). But we would have warnings during the build that list overridden keys. If the user objected to the overrides, they would be responsible for making the keys unique.

This is based on the assumption that the most common case will be to have 1 JSON file.

@dhimmel
Copy link
Member Author

dhimmel commented Nov 9, 2017

This is based on the assumption that the most common case will be to have 1 JSON file.

For my workflow, it'd probably be easiest to have 1 JSON file per notebook. This way the output of each notebook is self-sufficient... it doesn't need to get combined with other notebook outputs. The Sci-Hub Coverage Study currently has dozens of notebooks. So I think it's common that there will be a large number of JSON files.

I'll look into the possible implementations with argparse and report back. See also https://stackoverflow.com/a/11762020/4651668

@agitter
Copy link
Member

agitter commented Nov 9, 2017

Okay, that makes sense. In that case, you'll need a namespace (or something similar) no matter what. It will either be specified from the command line in the --template-variables-path argument or in the keys of the JSON files, right?

@dhimmel
Copy link
Member Author

dhimmel commented Nov 9, 2017

Seems like we'll want to set action='append':

This stores a list, and appends each argument value to the list. This is useful to allow an option to be specified multiple times.

It's possible we could support adding a namespace and defaulting to the global level. Good point @agitter that you could always namespace when you create the JSON files, although that requires a bit more planning sometimes.

@agitter
Copy link
Member

agitter commented Nov 10, 2017

Defaulting to a global namespace could be a good compromise. That way users with simple manuscripts and a single JSON file can continue using --template-variables-path as they do now with no change at the command line or in the manuscript files. Advanced users could specify namespaces.

A lazy solution could be to continue with a single namespace but throw an error during the build when there is a key collision. Or more selectively, a key collision in the subset of keys that are used in the manuscript, which may be hard to detect in advance unless Jinja2 offers support I'm not aware of. That way users would only need to manage namespaces in their JSON file keys when it became necessary.

dhimmel added a commit to dhimmel/manubot that referenced this issue Nov 29, 2017
Supports but does not require namespacing for backwards
compatability. If namespacing is not used, the JSON
payload must be a dictionary at the top-level.

Closes manubot#18
dhimmel added a commit to dhimmel/manubot that referenced this issue Nov 29, 2017
Supports but does not require namespacing for backwards
compatability. If namespacing is not used, the JSON
payload must be a dictionary at the top-level.

Closes manubot#18
dhimmel added a commit that referenced this issue Nov 29, 2017
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

No branches or pull requests

2 participants