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

DIG-1359: Documentation, error handling and code improvements for usability #42

Merged
merged 33 commits into from
Nov 22, 2023

Conversation

mshadbolt
Copy link
Contributor

@mshadbolt mshadbolt commented Nov 21, 2023

Relates to:

DIG-1343
DIG-1331

Summary of changes:

.gitignore:

  • add pycharm and libreoffice annoying files

CSVConvert.py

  • import csv and use to read csvs in read_mapping_template() so that quoted CSVs are read accurately
  • improve helper text for arguments
  • Some minor automatic formatting changes, typo fixes
  • Add check_for_sheet_inconsistencies() method to check that sheets identified in the mapping template are consistent with sheets given as input. (quick and dirty but not sure if best or most friendly way)
  • Exit the program if fatal errors are found

generate_mapping_docs.py

  • generates nicely readable documentation from docstrings in mappings.py and adds to mapping_functions.md
  • Could consider incorporating this into github actions

mapping_functions.md

  • add index of mapping functions automatically generated by generate_mapping_docs.py
  • Add further information about input to mapping functions

mappings.py

  • add more descriptive docstrings to all functions

schema.py

requirements.txt

  • add openapi-spec-validator and pdoc3

sample_inputs

  • add a functions file to the available sample_inputs

validate_coverage.py

  • remove unused args and imports
  • add exceptions for file not found or invalid json file

Test and moh templates

  • correct a few more indexed_on errors

Some concerns/questions/ideas:

  • It probably won't have consequences but it makes me nervous that mappings.float() overrides the inbuilt python method for casting as float
  • The method mappings.boolean() defaults to True if it doesn't match the listed options, this seems a little risky
  • Would it be worth implementing a more formal logging system so that messages can be split into those useful for a candig developer debugging versus a user trying to use the script?

mappings.py Outdated Show resolved Hide resolved
@mshadbolt mshadbolt marked this pull request as ready for review November 21, 2023 23:10
@mshadbolt mshadbolt requested a review from daisieh November 21, 2023 23:10
Copy link
Member

@daisieh daisieh left a comment

Choose a reason for hiding this comment

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

Teeny code changes requested in CSVConvert. Otherwise, looks awesome and thank you so much for cleaning up all of our nonsense!

CSVConvert.py Show resolved Hide resolved
CSVConvert.py Show resolved Hide resolved
mappings.py Outdated Show resolved Hide resolved
@mshadbolt mshadbolt requested a review from daisieh November 22, 2023 17:47
Copy link
Member

@daisieh daisieh left a comment

Choose a reason for hiding this comment

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

Looks awesome! Thanks!

@mshadbolt mshadbolt merged commit 0420284 into main Nov 22, 2023
1 check passed
@mshadbolt mshadbolt deleted the mshadbolt/docs-and-error-handling branch November 22, 2023 19:20
@mshadbolt mshadbolt changed the title Documentation, error handling and code improvements for usability DIG-1359: Documentation, error handling and code improvements for usability Nov 23, 2023
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.

2 participants