Add some features #86
Conversation
…'" error Changing the working directory to the app's directory to avoid the "ImportError: No module named 'server'" error be begginner in Python.
…'" error Changing the working directory to the app's directory to avoid the "ImportError: No module named 'server'" error be begginner in Python.
Adding feature to customize the listening interface via the configuration file in order to allow the access only via localhost (or 127.0.0.1), via a specific interface (ie : 192.168.1.25) or, all interface (0.0.0.0).
Add feature to allow the possibility to use variables in the path of the output files which has been updated by the arguments specified before launching the script.
csphoenix1
changed the title
|
Hi @csphoenix1 , thanks for the features, they look useful. I have some comments on replacing variables, please check my code review comments. If you want, I can do all requested review changes myself. By the way, do you use output files feature? I added it just in case and don't use it myself. |
src/model/model_helper.py
Outdated
| if argument in arguments: | ||
| value = arguments[argument] | ||
| output_file = output_file.replace("$$" + argument, value) | ||
| output_files[i] = output_file |
There was a problem hiding this comment.
Could we return modified array? Instead of changing internal state of the config?
Also I would suggest to move the function to features/file_download_feature.py and call it directly inside prepare_downloadable_files
There was a problem hiding this comment.
Yes good idead, I will modified it.
src/server.py
Outdated
| @@ -470,6 +470,8 @@ def post(self): | |||
| respond_error(self, 400, 'Received invalid parameters') | |||
| return | |||
|
|
|||
| model_helper.update_output_files_with_vars(config.output_files, execution_info.param_values) | |||
There was a problem hiding this comment.
Please read my comment regarding moving the call to FinishListener and file_download_feature
There was a problem hiding this comment.
Yes no problem ;)
src/model/model_helper.py
Outdated
| @@ -27,6 +28,18 @@ def is_empty(value): | |||
| return (not value) and (value != 0) and (value != False) | |||
|
|
|||
|
|
|||
| def update_output_files_with_vars(output_files, arguments): | |||
| for i, output_file in enumerate(output_files): | |||
| regex = "\$\$([a-zA-Z0-9-_]+)" # Syntaxe is : $$my_variable (with two '$') | |||
There was a problem hiding this comment.
I would suggest to change internal loop a bit: we can iterate over all arguments and build regex based on it:
e.g.
for i, output_file in enumerate(output_files):
for argument in arguments:
output_file = re.sub('\$\$' + argument, arguments[argument], output_file)
Also secure arguments should be excluded
One more thing/question. $$ syntax is already used for environment variables in couple of places. What do you think, won't reusing the syntax cause any confusion? As for me, it shouldn't be a problem
There was a problem hiding this comment.
+1 about the loop.
About the syntaxe, i didn't know it was used for environment variables.
To avoid bad surprise, it would be better if we use 2 differents syntaxes. So, since the "$$" syntaxe is already used, we could use "$$$" or "$"? What do you think about? "$$$" is not use in contrary to "$" which is used by PHP.
Patch1 branch before bugy's validation
Taking in count bugy comment's
|
Also, you will have to update the Wiki :)
|
src/server.py
Outdated
| @@ -361,6 +361,14 @@ def finished(self): | |||
|
|
|||
| output_logger.close() | |||
|
|
|||
| # What's the diffrence between "get_config()" and "config" ? | |||
There was a problem hiding this comment.
There is no difference.
I've created a getter method because I was used to it in Java. However in python it's not needed and you can safely access class fields directly. So, please use "config.output_files"
src/server.py
Outdated
| # process_wrapper.get_config().output_files = file_download_feature.update_output_files_vars_with_args( | ||
| # process_wrapper.get_config().output_files, | ||
| # process_wrapper.execution_info.param_values)# | ||
| process_wrapper.config.output_files = file_download_feature.update_output_files_vars_with_args( |
There was a problem hiding this comment.
could you move this call inside prepare_downloadable_files? So something like:
def prepare_downloadable_files(...):
output_files = config.output_files
if not output_files:
return []
output_files = update_output_files_vars_with_args(...)
|
Regarding wiki update - thanks, I'll do it after merging your changes (both for output files and address). However I'm a bit confused - in your proposed wiki statement you used "interface" name, however according to the code it should be "address". Do I miss something? |
Moving "update_output_files_vars_with_args" call inside "prepare_downloadable_files".
|
Hi @bugy ! PS : I've created some issue about involves that it could be cool :) |
|
Hi @csphoenix1 , thank you for the changes and for new feature requests! :) |
No description provided.