-
Notifications
You must be signed in to change notification settings - Fork 0
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
task/WG-96: support questionnaires with assets #131
task/WG-96: support questionnaires with assets #131
Conversation
This reverts commit 698da33.
Ensure file name is correct and a preview image is created. Also, store info on assets' geolocation
geoapi/tasks/external_data.py
Outdated
# determine full path for this asset and add to list | ||
additional_file_path = current_file_path.with_name(asset["filename"]) | ||
additional_files_to_get.append(AdditionalFile(path=additional_file_path, required=True)) | ||
logger.info(f'{len(additional_files_to_get)} assets were found for rq file {current_file.filename}') |
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.
There is an inconsistency with using single and double quotes through this file. Could we normalize that as much as possible to one of the two?
Compare lines 88, 93, 94, 99, 107, 122, etc.
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.
I don't know that TACC WMA has a preference but I tend to prefer this usage:
- raw string literals for regular expressions
- single quotes for small symbol-like strings
- double quotes around strings that are used for interpolation or that are natural language messages (break the rules as needed if the strings contain quotes to escape)
- triple double quotes for docstrings
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.
I took a stab at this in 65eeef8. let me know if that matches up with your 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.
Approved with a few recommendations on naming choices and style consistency.
geoapi/tasks/external_data.py
Outdated
# determine full path for this asset and add to list | ||
additional_file_path = current_file_path.with_name(asset["filename"]) | ||
additional_files_to_get.append(AdditionalFile(path=additional_file_path, required=True)) | ||
logger.info(f'{len(additional_files_to_get)} assets were found for rq file {current_file.filename}') |
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.
I don't know that TACC WMA has a preference but I tend to prefer this usage:
- raw string literals for regular expressions
- single quotes for small symbol-like strings
- double quotes around strings that are used for interpolation or that are natural language messages (break the rules as needed if the strings contain quotes to escape)
- triple double quotes for docstrings
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.
LGTM! Thanks for straightening out the syntax on quote usage!
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.
LGTM
Overview:
Support assets in questionnaires.
This PR imports the .rq file and all the associated asset images.
Related Jira tickets:
Testing Steps: