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

Add typing #716

Merged
merged 15 commits into from
Apr 4, 2024
Merged

Add typing #716

merged 15 commits into from
Apr 4, 2024

Conversation

Viicos
Copy link
Contributor

@Viicos Viicos commented Jan 21, 2024

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
Copy link
Collaborator

Overall looks good!

We also need to add mypy to CI

modeltranslation/utils.py Outdated Show resolved Hide resolved
@Viicos
Copy link
Contributor Author

Viicos commented Jan 24, 2024

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

FALLBACK_LANGUAGES = getattr(settings, "MODELTRANSLATION_FALLBACK_LANGUAGES", (DEFAULT_LANGUAGE,))
if isinstance(FALLBACK_LANGUAGES, (tuple, list)):
FALLBACK_LANGUAGES = {"default": tuple(FALLBACK_LANGUAGES)}

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:

fallback_for_lang = override.get(lang, settings.FALLBACK_LANGUAGES.get(lang, ()))
fallback_def = override.get("default", settings.FALLBACK_LANGUAGES["default"])

@last-partizan
Copy link
Collaborator

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
Copy link
Contributor Author

Viicos commented Feb 9, 2024

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
Copy link
Collaborator

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.

pyproject.toml Outdated Show resolved Hide resolved
@Viicos
Copy link
Contributor Author

Viicos commented Feb 27, 2024

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
Copy link
Collaborator

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
Copy link
Contributor Author

Viicos commented Feb 27, 2024

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)

Makefile Outdated Show resolved Hide resolved
@last-partizan
Copy link
Collaborator

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

@last-partizan
Copy link
Collaborator

Looks like we need to apply ruff format.

pyrightconfig.json Outdated Show resolved Hide resolved
@Viicos
Copy link
Contributor Author

Viicos commented Mar 6, 2024

I still need to look into test failures

@Viicos Viicos changed the title WIP: Add typing Add typing Mar 6, 2024
@Viicos
Copy link
Contributor Author

Viicos commented Apr 2, 2024

@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
Copy link
Collaborator

@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
Copy link
Contributor Author

Viicos commented Apr 3, 2024

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

@last-partizan last-partizan merged commit a9e95e8 into deschler:master Apr 4, 2024
13 checks passed
@last-partizan
Copy link
Collaborator

Thanks, merged!

@Viicos Viicos deleted the typing branch April 4, 2024 08:31
@Viicos
Copy link
Contributor Author

Viicos commented Apr 4, 2024

Thanks!

@Viicos
Copy link
Contributor Author

Viicos commented Apr 4, 2024

Will add the py.typed marker in a new PR

@Viicos Viicos mentioned this pull request Apr 4, 2024
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.

Typing support
2 participants