-
Notifications
You must be signed in to change notification settings - Fork 258
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
Add typing #716
Add typing #716
Conversation
Overall looks good! We also need to add mypy to CI |
django-modeltranslation/modeltranslation/settings.py Lines 40 to 42 in 2f256fa
Even by annotating This then causes issues when django-modeltranslation/modeltranslation/utils.py Lines 126 to 127 in 2f256fa
|
I don't see any annotation for it. Maybe you're annotated another variable? |
This ended up being a bit bigger than I expected, but I'm pretty surprised with the reasonable amount of type ignores I had to use (around 35, considering a couple of them will be unnecessary with a new I still had to globally ignore some codes per modules, because it was just counter productive to actually enable them (we need to remember the primary goal is for end users and also when developing the library. I have to say adding type hints really helped to understand what |
I was probably thinking that it's still WIP, because of the title. But looks like it's ready? I'm gonna review this, you need to resolve small merge conflict in lockfile, and we can merge this. |
One mypy failure should remain: modeltranslation/admin.py: note: In member "_get_declared_fieldsets" of class "TranslationBaseModelAdmin":
modeltranslation/admin.py:51: error: Argument 1 to "replace_orig_field" of "TranslationBaseModelAdmin" has incompatible type
"Sequence[Union[str, Sequence[str]]]"; expected "Iterable[str]" [arg-type]
return [(None, {"fields": self.replace_orig_field(self.get_fields(request, obj))})]
^~~~~~~~~~~~~~~~~~~~~~~~~~~~~ and the current signature of def replace_orig_field(self, option: Iterable[str]) -> _ListOrTuple[str]: ... Not sure what to do with this one, I don't know if |
mypy is right here, take a look at the function dosctring, it really accepts |
My bad I added a wrong signature to |
Now, please fix linter errors and merge latest changes from master into your branch. |
Looks like we need to apply |
I still need to look into test failures |
@last-partizan Sorry if this took some time, I was struggling to find some free time working on it. But this is now done I believe |
@Viicos don't worry, we're all unpaid volunteers here, and nobody expects any deadlines :) Please, merge master into this branch, and resolve conflicts. |
Rebased (merging wasn't possible for some reason (says I'm up to date), the branch is in a weird state) |
Thanks, merged! |
Thanks! |
Will add the |
Fixes #715
I've made use of
typing-extensions
. I know some people doesn't like to add an extra dependency purely for typing, butasgiref
(a Django dependency) requires it anyway.I made a first
mypy
run, and it was looking alright, I'll finish adding typing when I get some more time.