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

test: fix type hinting on test_jujuversion tests #1011

Merged
merged 6 commits into from
Sep 22, 2023

Conversation

tonyandrewmeyer
Copy link
Contributor

@tonyandrewmeyer tonyandrewmeyer commented Sep 22, 2023

JujuVersion's comparisons can be done against strings as well as other JujuVersions, so the type hints should reflect that.

For mocking the os.environ response, the type is dict(str, str) but I think to declare that we would need to define the dict, e.g:

    _mockenv : typing.Dict[str, str] = {}
    @unittest.mock.patch("os.environ", new=_mockenv)

And that looks considerably messier to my eyes and doesn't seem to really add value (so I've ignored the warning).

Partially addresses #1007

If a string is passed, we'll convert it to a JujuVersion, so the correct type is JujuVersion|str.
This is a dict(str, str) but it's not worth defining that and making the decorator call messy in this specific test case.
Copy link
Collaborator

@benhoyt benhoyt left a comment

Choose a reason for hiding this comment

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

LGTM. Agreed the typing for patch/new is messy! new={'': ''} is shorter but confusing. So I think type: ignore is reasonable here.

Let's just do the thing I mentioned in the other PR about adding the files to pyproject.toml as we go so that's up to date. When we finish, we can replace the individual names with test/*.py.

Maybe break include onto multiple lines to avoid so many conflicts.

@benhoyt benhoyt merged commit 8c09aa2 into canonical:main Sep 22, 2023
18 checks passed
@tonyandrewmeyer tonyandrewmeyer deleted the pyright-test_jujuversion-1007 branch September 22, 2023 06:04
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants