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

Improve type hinting #261

Merged
merged 5 commits into from Dec 13, 2021
Merged

Conversation

SmileyChris
Copy link
Contributor

make/prepare now return the correct type depending on _quantity

`make`/`prepare`  now return the correct type depending on `_quantity`
Copy link
Member

@berinhard berinhard left a comment

Choose a reason for hiding this comment

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

Thanks for the contribution, but the CI is breaking due to a broken from __future__ import.

model_bakery/baker.py Outdated Show resolved Hide resolved
@timjklein36
Copy link
Collaborator

These changes actually introduce some breaks on my local machine for mypy. See output below:

# python -m mypy model_bakery
model_bakery/baker.py:114: error: Need type annotation for "baker"
model_bakery/baker.py:229: error: Incompatible types in assignment (expression has type "Optional[Type[_M]]", variable has type "Type[_M]")
model_bakery/baker.py:231: error: Incompatible types in assignment (expression has type "None", variable has type "Type[_M]")
model_bakery/baker.py:497: error: Name "instance" already defined on line 484
model_bakery/recipe.py:13: error: Need type annotation for "finder"
Found 5 errors in 2 files (checked 10 source files)

Compared with no failures against main locally.

@berinhard @amureki It seems like the tox-linter GH Action succeeded here, but it failed on my local machine. Any reason that you can think of for the inconsistency?

@SmileyChris
Copy link
Contributor Author

SmileyChris commented Nov 3, 2021

Fixed mypy issues (and to follow up on what @timjklein36 was saying, the gh linter action definitely isn't working correctly -- if you drill into the output of the tox step, it's not doing the work it should be)

Copy link
Member

@berinhard berinhard left a comment

Choose a reason for hiding this comment

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

Thanks for the updates @SmileyChris!

@timjklein36 I did run the mypy check locally and it's passsing:

$ python -m mypy model_bakery
Success: no issues found in 10 source files

Is it failing for you even after the updates on this PR?

Copy link
Collaborator

@timjklein36 timjklein36 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 some questions about the implementation. Some of my confusion may come from not being very familiar with generic types in Python, so I apologize if that is the case.

model_bakery/baker.py Outdated Show resolved Hide resolved
@timjklein36
Copy link
Collaborator

@berinhard mypy is now succeeding locally for me. Once I get clarification on my questions above, I will be prepared to accept the PR.

@SmileyChris
Copy link
Contributor Author

Renamed. The same overload patterns could also be applied to recipes, but for the sake of smaller PRs, I can submit that separately.

@amureki
Copy link
Collaborator

amureki commented Nov 5, 2021

@berinhard @amureki It seems like the tox-linter GH Action succeeded here, but it failed on my local machine. Any reason that you can think of for the inconsistency?

I guessed that it is because of different Python versions in GitHub Actions config and tox. Here is a separate PR to fix it:
#262

@amureki amureki merged commit fea8c21 into model-bakers:main Dec 13, 2021
@SmileyChris SmileyChris deleted the type-hint-overloads branch December 14, 2021 21:06
@belkka
Copy link

belkka commented Dec 17, 2021

I've come here being ready to make exaclty same PR. Nice to see it's already done and merged! 🎉

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.

None yet

5 participants