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

task/WG-96: support questionnaires with assets #131

Conversation

nathanfranklin
Copy link
Collaborator

@nathanfranklin nathanfranklin commented Jun 29, 2023

Overview:

Support assets in questionnaires.

This PR imports the .rq file and all the associated asset images.

Related Jira tickets:

Testing Steps:

  1. Test with related hazmapper branch task/WG-96 add assets to original questionnaire code hazmapper#144
  2. Load questionnaire from the example in https://www.designsafe-ci.org/data/browser/projects/8131157524548227566-242ac117-0001-012/ "PRJ-4019 | Questionnaire_Example"
  3. ensure the questionnaire is loaded
  4. and can be viewed.

@nathanfranklin nathanfranklin marked this pull request as draft June 29, 2023 20:19
@nathanfranklin nathanfranklin marked this pull request as ready for review October 6, 2023 15:38
@nathanfranklin nathanfranklin changed the base branch from master to feature/questionnaire October 9, 2023 16:50
@nathanfranklin nathanfranklin changed the base branch from feature/questionnaire to master October 9, 2023 16:57
@nathanfranklin nathanfranklin changed the base branch from master to feature/questionnaire October 9, 2023 16:58
@nathanfranklin nathanfranklin changed the base branch from feature/questionnaire to DES-1522 October 9, 2023 17:03
@nathanfranklin nathanfranklin changed the base branch from DES-1522 to master October 9, 2023 17:03
@nathanfranklin nathanfranklin changed the base branch from master to feature/questionnaire October 9, 2023 17:04
@nathanfranklin nathanfranklin changed the base branch from feature/questionnaire to master October 9, 2023 17:06
@nathanfranklin nathanfranklin changed the base branch from master to feature/questionnaire October 9, 2023 17:08
geoapi/services/features.py Outdated Show resolved Hide resolved
# 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}')
Copy link
Contributor

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.

Copy link
Contributor

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

Copy link
Collaborator Author

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.

Copy link
Contributor

@taoteg taoteg left a 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.

# 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}')
Copy link
Contributor

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

geoapi/tests/fixtures/questionnaire.rq Outdated Show resolved Hide resolved
geoapi/tests/external_data_tests/test_external_data.py Outdated Show resolved Hide resolved
geoapi/tests/external_data_tests/test_external_data.py Outdated Show resolved Hide resolved
geoapi/tests/external_data_tests/test_external_data.py Outdated Show resolved Hide resolved
Copy link
Contributor

@taoteg taoteg left a 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!

Copy link
Contributor

@sophia-massie sophia-massie left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

LGTM

@nathanfranklin nathanfranklin merged commit e3ffcae into feature/questionnaire Oct 16, 2023
3 checks passed
@nathanfranklin nathanfranklin deleted the task/WG-96-support-questionnaires-with-assets branch October 16, 2023 15:29
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.

3 participants