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

update dependencies #91

Merged
merged 1 commit into from
Jul 11, 2023
Merged

update dependencies #91

merged 1 commit into from
Jul 11, 2023

Conversation

ghukill
Copy link
Contributor

@ghukill ghukill commented Jul 11, 2023

Helpful background context

  • adding .idea to .gitignore for Pycharm IDE project dir (similar to .vscode)
  • updating dependencies
  • click updated from 8.1.3 to 8.1.4; changed order of decorators to address mypy linting error

How can a reviewer manually see the effects of these changes?

Tested that changing click decorator order doesn't affect invoking:

pipenv run transform -i "s3://timdex-extract-dev-222053980223/alma/alma-2023-05-04-daily-extracted-records-to-index.xml" -o "output/test.json" -s alma

Developer

  • All new ENV is documented in README
  • Stakeholder approval has been confirmed (or is not needed)

Code Reviewer

  • The commit message is clear and follows our guidelines
    (not just this pull request message)
  • There are appropriate tests covering any new functionality
  • The documentation has been updated or is unnecessary
  • The changes have been verified
  • New dependencies are appropriate or there were no changes

Includes new or updated dependencies?

YES

Why these changes are being introduced:
* adding .idea to .gitignore for Pycharm IDE project dir (similar to .vscode)
* updating dependencies
* click updated from 8.1.3 to 8.1.4; changed order of decorators to address mypy linting error
@ghukill ghukill force-pushed the dependencies-update branch from cb72994 to 4fe1236 Compare July 11, 2023 20:39
Copy link
Contributor

@ehanson8 ehanson8 left a comment

Choose a reason for hiding this comment

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

Looks good and works as expected!

@ghukill ghukill merged commit e9c5212 into main Jul 11, 2023
@ghukill
Copy link
Contributor Author

ghukill commented Jul 11, 2023

After work was completed and some more digging, found this issue with click library: pallets/click#2558

While the reordering of the decorators does pass mypy linting at this time, it's probably something to keep an eye on.

ghukill added a commit that referenced this pull request Jul 18, 2023
Why these changes are being introduced:
* In PR #91, while updating dependencies, `click` and `mypy` had a conflict.
* A temporary workaround was put into place that reordered the `click` decorators, but this deviated from other projects.
* Now that `click` has updated again, that workaround is no longer needed.

How this addresses that need:
* Reverting the order of `click` decorators re-aligns with other projects.
* Dependencies are updated.

Side effects of this change:
* None

Relevant ticket(s):
* None
ghukill added a commit that referenced this pull request Jul 18, 2023
* Update type hints in helpers.write_timdex_records_to_json

Why these changes are being introduced:
* After some refactoring in 07/2022 that added an iterator pattern to Transformer, it looks as though downstream type hints were not updated.
* While this did not affect runtime, it made reading the code somewhat confusing to someone new to the codebase.

How this addresses that need:
* Update the `records` input argument of `helpers.write_timdex_records_to_json()` from `Iterator[TimdexRecord]` to `Transformer`
* rename `records` to `transformer_instance`, matching variable names from calling `cli.py`
* Add explicit `TimdexRecord` type hint for result of `next(transformer_instance)`

Side effects of this change:
* None

Relevant ticket(s):
* None

Additional maintenance:
* Updated syntax for isort command in Makefile
  * Based on new results from isort, update import order in helpers.py
* update dependencies and revert click workaround
  * In PR #91, while updating dependencies, `click` and `mypy` had a conflict.
  * A temporary workaround was put into place that reordered the `click` decorators, but this deviated from other projects.
  * Now that `click` has updated again, that workaround is no longer needed.
  * Resolves #92
@ghukill ghukill deleted the dependencies-update branch July 20, 2023 20:21
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