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

Borrow API update & Security #13

Open
wants to merge 3 commits into
base: master
Choose a base branch
from

Conversation

joeflack4
Copy link
Contributor

I will be using this branch to deploy borrow web. I think it is solid enough to merge. But we can review and change things if you would like.

Security

  • Security updates via dependency version up.

Developer updates

The following was done to improve compatibility, especially with ArgparseToWebform.

  • Exposed ArgumentParser object as 'parser' to Borrow API by moving it to global scope of module.
  • Renamed CLI option 'xlsxfile' to 'xlsxfiles' to make consistent with the Python API.
  • Altered Python API borrow() function definition. Removed unneeded *. Changed optional params to default to None or False. Strongly coupled CLI and Python API parameter interface by requiring CLI parameters passed to be exactly the same, via dynamically loading list of attribute names from the borrow() function definition.

- Exposed ArgumentParser object as 'parser' to Borrow API by moving it to global scope of module. Rationale is to allow for use with ArgeparseToWebform.
- Renamed CLI option 'xlsxfile' to 'xlsxfiles' to make consistent with the Python API.
- Altered Python API borrow() function definition. Removed unneeded *. Changed optional params to default to None or False. Strongly coupled CLI and Python API parameter interface by requiring CLI parameters passed to be exactly the same, via dynamically loading list of attribute names from the borrow() function definition.
@joeflack4 joeflack4 requested a review from jkpr July 26, 2019 00:35
Copy link
Contributor

@jkpr jkpr left a comment

Choose a reason for hiding this comment

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

I have made some comments on your changes. Please review and I look forward to your feedback ☄️

pmix/borrow.py Show resolved Hide resolved
pmix/borrow.py Outdated Show resolved Hide resolved
pmix/borrow.py Outdated Show resolved Hide resolved
@joeflack4
Copy link
Contributor Author

@jkpr Thanks for the feedback! I'll prioritize this after the Ethiopia forms, but eager to get back to this.

@joeflack4
Copy link
Contributor Author

@jkpr Requested changes have been made!

pmix/borrow.py Outdated Show resolved Hide resolved
- Refactored borrow_cli() function to be much cleaner.
- Re-added leading * to borrow() function definition to require keyword args.
- Removed parser obj from global module variable in favor of cli_parser() constructor.
- Updated .gitignore
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