Add type support and mypy checking #100
Conversation
There was a problem hiding this comment.
Woah, that is massive work here @palfrey ! Very well done!
I am planning on taking a deeper look and play around it before I'd be fine moving forward, I hope this is fine with you.
In general, I am all up for this change 👍
On a sidenote, there was a misconfiguration in GitHub actions, which led to test checks not running.
I fixed it, hopefully rebasing will trigger them here.
Best,
Rust
model_bakery/random_gen.py
Outdated
| def num_as_str(x): | ||
| return "".join([str(randint(0, 9)) for _ in range(x)]) |
There was a problem hiding this comment.
This function also should be covered, I guess.
There was a problem hiding this comment.
I've locked down the mypy settings to disallow untyped calls, which flagged this one and various others I've now fixed.
model_bakery/random_gen.py
Outdated
| return now().time() | ||
|
|
||
|
|
||
| def gen_string(max_length): | ||
| @action_generator |
There was a problem hiding this comment.
Not sure I follow exactly why we are putting this workaround under certain methods, which don't have direct attributes. But I need to take a deeper look here and will play with the code deeper.
There was a problem hiding this comment.
This annotation is on all the generators that have .required set. We could probably add it to the others, but it wasn't needed for type checking, so I hadn't done so yet
There was a problem hiding this comment.
I have mixed feelings about this. For now, for example, if an user wants to define a custom generator function, they'll only have to do something like:
def custom_gen_func():
return "random value"
baker.generators.add('test.generic.fields.CustomField', custom_gen_func)
By introducing this new @action_generator the user will have an extra step that's to remind to decorate the function with it. Also, I'm now sure how will it work with existing test.
There was a problem hiding this comment.
Hadn't thought about the external generator case. Right, I've killed off action_generator entirely. It was a nice idea, but the external case makes it unusable. I've added ignores for the existing usages of all the properties so mypy won't complain about them (which reduces the checking a bit, but still better to ignore some things and check others than not check at all)
No worries. I've fixed the build issues. There's still a few instances of |
There was a problem hiding this comment.
@palfrey I REALLY liked your PR and appreciate the effort you've put in it. But, if is ok for you, I'd like more time to think about it. Honestly, I'm not one of the hugest fans of static type checking in Python and I'm not sure yet the benefits it will bring to model-bakery given the fact that this would introduce backwards incompatibilities when it comes to custom generators.
As I said, I just need more time to think, and also would like to hear from @amureki and @anapaulagomes what they thing about it as well. Maybe we can wait to merge this PR for a major release since it can have an annoying impact in projects which uses custom generators. And maybe people will be annoyed by something they didn't want first that's type checking.
Please, don't take me wrong, I really appreciated your PR and I hope I'm not being rude. But I think this PR needs a more open discussion before we merge it in.
model_bakery/random_gen.py
Outdated
| return now().time() | ||
|
|
||
|
|
||
| def gen_string(max_length): | ||
| @action_generator |
There was a problem hiding this comment.
I have mixed feelings about this. For now, for example, if an user wants to define a custom generator function, they'll only have to do something like:
def custom_gen_func():
return "random value"
baker.generators.add('test.generic.fields.CustomField', custom_gen_func)
By introducing this new @action_generator the user will have an extra step that's to remind to decorate the function with it. Also, I'm now sure how will it work with existing test.
tests/test_extending_bakery.py
Outdated
| from tests.generic.models import Person | ||
|
|
||
|
|
||
| @action_generator |
There was a problem hiding this comment.
This is an example of what I was trying to say in my previous comment. The existing custom generators API will break =/
|
@palfrey I hope you don't mind it, but I brought this PR to Twitter so we can hear more opinions from other pythonistas about the static type checking subject as well. One think I didn't know and I really liked is that we can use files with the
Are you familiar with |
No worries. Glad to see review of PRs I submit at all! Far too many projects sit on things for months/years, so fast review is good :)
I'm familiar with it, but mostly from a PoV of stubs in projects for third-party code e.g. I have some model_bakery .pyi stubs in a project of mine at the moment. I see from that example how it could work, but I'm not in favour of it for one major reason. I regard typing data for Python as a lot like docs, as in the best place for them to be kept up-to-date is next to the actual code. Putting it in a different file, even if it's literally next to it in a directory feels wrong to me. I'm willing to split this out to that if you really want to, but I'd regard it as a bad idea. |
Co-authored-by: Bernardo Fontes <bernardoxhc@gmail.com>
|
The build now appears to be failing for reasons that appear to have nothing to do with the work here. @berinhard Any thoughts? |
Appears to be something flaky in test teardown which has now not triggered... |
There was a problem hiding this comment.
Hi @palfrey thanks for your PR updates! I'm really glad you've considered my opinions and rolled back the decorator for the generators function. About the .pyi file, I agree with you that it can be problematic to maintain the same types as the code does because they're separated, but I'm still in favor for this approach.
Anyway, I don't want to waste your PR or force you to rewrite everything. If you are up to it, we can:
- Merge in this PR;
- Open an issue to refactor the type hinting to use
.pyi;
So, we can document this in the project and, if you have some time and will in the future, you're more than welcome to help us with the refactoring. If I'm not asking to much and if you agree with this, can you help us by opening this refactoring issue?
About the CI breaking, we have this check to force all PRs to update our changelog. It keeps it easier for us to release new versions of model-bakery without worrying if the changes were documented somewhere. So, it's just a matter of adding an entry to the Added session and this should be good do go =)
I'll wait for an extra review from @anapaulagomes or @amureki to merge this in and release a new model-bakery version so you can use it on your projects, ok?
WFM. Issue for pyi is #108.
I'm unlikely to help with the refactoring, both because I disagree with the approach and I've got a few other things I need to deal with. Doesn't mean I won't be back around to provide other fixes I find to stuff, but we'll see :)
Done
👍 |
|
I'll look into this until the weekend. 🙏🏽 |
Fixes #98