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

revert changes to click decorators after click/mypy conflicts resolved #92

Closed
ghukill opened this issue Jul 13, 2023 · 1 comment · Fixed by #96
Closed

revert changes to click decorators after click/mypy conflicts resolved #92

ghukill opened this issue Jul 13, 2023 · 1 comment · Fixed by #96
Assignees

Comments

@ghukill
Copy link
Contributor

ghukill commented Jul 13, 2023

While updating dependencies, PR #91 also reworked the order of click decorators to address a new, external conflict between click==8.1.3 and mypy==1.4.1.

This underlying conflict was discovered after the code change, and it's assumed a fix will be rolled out to click in the near future that will allow the previous decorator order while also aligning with mypy linting, as many others have encountered this as well.

Proposing to revert the click decorators to match other projects (with @click.command as the top-most decorator, with others following) once it's been determined that click and mypy are aligned.

Current:

@click.option(
    "-i",
    "--input-file",
    required=True,
    help="Filepath for harvested input records to transform",
)
@click.option(
    "-o",
    "--output-file",
    required=True,
    help="Filepath to write output TIMDEX JSON records to",
)
@click.option(
    "-s",
    "--source",
    required=True,
    type=click.Choice(list(SOURCES.keys()), case_sensitive=False),
    help="Source records were harvested from, must choose from list of options",
)
@click.option(
    "-v", "--verbose", is_flag=True, help="Pass to log at debug level instead of info"
)
@click.command()
def main(source, input_file, output_file, verbose):
...

Proposed reverted ordering, matching convention in other projects:

@click.command()
@click.option(
    "-i",
    "--input-file",
    required=True,
    help="Filepath for harvested input records to transform",
)
@click.option(
    "-o",
    "--output-file",
    required=True,
    help="Filepath to write output TIMDEX JSON records to",
)
@click.option(
    "-s",
    "--source",
    required=True,
    type=click.Choice(list(SOURCES.keys()), case_sensitive=False),
    help="Source records were harvested from, must choose from list of options",
)
@click.option(
    "-v", "--verbose", is_flag=True, help="Pass to log at debug level instead of info"
)
def main(source, input_file, output_file, verbose):
...
@ehanson8
Copy link
Contributor

https://github.com/MITLibraries/alma-sapinvoices is also affected by this issue and the click version will need to be unpinned when the incompatibility is resolved

ghukill added a commit that referenced this issue 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
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 a pull request may close this issue.

2 participants