deschler / django-modeltranslation

Translates Django models using a registration approach.
BSD 3-Clause "New" or "Revised" License
1.34k stars 257 forks source link

Add typing #716

Closed Viicos closed 3 months ago

Viicos commented 5 months ago

Fixes #715

I've made use of typing-extensions. I know some people doesn't like to add an extra dependency purely for typing, but asgiref (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.

last-partizan commented 5 months ago

Overall looks good!

We also need to add mypy to CI

Viicos commented 5 months ago

mypy is not "smart" enough to infer FALLBACK_LANGUAGES as being dict[str, tuple[str, ...]]:

https://github.com/deschler/django-modeltranslation/blob/2f256fa34920dc6174178b2ccd182404ac080dee/modeltranslation/settings.py#L40-L42

Even by annotating FALLBACK_LANGUAGES in the first line. pyright seems to handle it fine. I'll try to find a workaround but that might involve some code changes in the settings module.

This then causes issues when FALLBACK_LANGUAGES is being used:

https://github.com/deschler/django-modeltranslation/blob/2f256fa34920dc6174178b2ccd182404ac080dee/modeltranslation/utils.py#L126-L127

last-partizan commented 5 months ago

mypy is not "smart" enough to infer FALLBACK_LANGUAGES as being dict[str, tuple[str, ...]]: Even by annotating FALLBACK_LANGUAGES in the first line.

I don't see any annotation for it. Maybe you're annotated another variable?

Viicos commented 4 months ago

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 django-stubs release).

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 modeltranslation was doing in some cases).

last-partizan commented 4 months ago

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.

Viicos commented 4 months ago

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 replace_orig_field is:

def replace_orig_field(self, option: Iterable[str]) -> _ListOrTuple[str]: ...

Not sure what to do with this one, I don't know if get_fields can really return Sequence[Sequence[str]]?

last-partizan commented 4 months ago

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 replace_orig_field is:

def replace_orig_field(self, option: Iterable[str]) -> _ListOrTuple[str]: ...

Not sure what to do with this one, I don't know if get_fields can really return Sequence[Sequence[str]]?

mypy is right here, take a look at the function dosctring, it really accepts Sequence[Sequence[str] | str]

Viicos commented 4 months ago

My bad I added a wrong signature to replace_orig_field (it actually handles both sequences of str and nested sequences of sequences of str)

last-partizan commented 4 months ago

Now, please fix linter errors and merge latest changes from master into your branch.

last-partizan commented 4 months ago

Looks like we need to apply ruff format.

Viicos commented 4 months ago

I still need to look into test failures

Viicos commented 3 months ago

@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

last-partizan commented 3 months ago

@Viicos don't worry, we're all unpaid volunteers here, and nobody expects any deadlines :)

Please, merge master into this branch, and resolve conflicts.

Viicos commented 3 months ago

Rebased (merging wasn't possible for some reason (says I'm up to date), the branch is in a weird state)

last-partizan commented 3 months ago

Thanks, merged!

Viicos commented 3 months ago

Thanks!

Viicos commented 3 months ago

Will add the py.typed marker in a new PR