-
Notifications
You must be signed in to change notification settings - Fork 13
standalone template retriever #95
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
Conversation
oesteban
left a comment
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Thanks a lot for this contribution, and apologies for having failed to spot it until now.
I have added some comments. Please run black to style the code with templateflow's standards.
|
|
||
| [options.entry_points] | ||
| console_scripts = | ||
| templateflow_get=templateflow.templateflow_get:main |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
| templateflow_get=templateflow.templateflow_get:main | |
| templateflow=templateflow.templateflow_get:main |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
hum I'm affraid it will collapse will templateflow name space....
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
have you tried? because it won't...
| @@ -0,0 +1,197 @@ | |||
| #!/opt/gensoft/exe/TemplateFlow_pyclient/0.8.0/venv/bin/python3.8 | |||
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This should have the standard apache header (which implicitly means you accept your contribution is covered by the license).
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
my bad, I should have cleaned this sehbang.
I will
|
|
||
| import pprint | ||
|
|
||
| TOOL_VERSION = '0.1' |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
why having a different version for this module?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
first itération of the "getter" tool, I will comply with the module version instead
| m_group.add_argument('-o', '--tf-cache', | ||
| action='store', | ||
| default='unset', | ||
| help='Directory to save templates in. default `${HOME}/.cache/templateflow`') |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
this works around the TEMPLATEFLOW_HOME environment variable definition. please remove the option to do that.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
not sure to understand
this will allow users to grab the templates where they want.
maybee I did not cactch the templateflow philosophy
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This PR should only give access to the templateflow API. There is no function to change TEMPLATEFLOW_HOME within the API, and the CLI shouldn't either.
If a user wants to change where templates are cached, they should set the environment variable themselves.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
OK got the point. I will adapt regarding this point.
thanks for the feedback
Eric
| if opts.tf_cache != 'unset': | ||
| #---- TemplatFlow cache dir was specified, export it | ||
| print("saving to ---------->", opts.tf_cache) | ||
| print("saving to ---------->", os.path.abspath(opts.tf_cache)) | ||
| os.environ['TEMPLATEFLOW_HOME'] = os.path.abspath(opts.tf_cache) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
this should be managed by templateflow
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
seems to me I was lazy and copied this spnippet from the docs or examples .
|
Hi @EricDeveaud, do you have the bandwidth to take this over the finish line? |
|
hello, unfortunatly currently not. sorry for the delay Eric |
|
Closing in favor of #123. |
we had the need to provide a standalone CLI utility scripts for template download.
this basic script wraps the python module and provide a CLI interface
maybee it will be usefull for templateflow users
regards
Eric