DisnakeDev / disnake

An API wrapper for Discord written in Python.
https://docs.disnake.dev
MIT License
721 stars 137 forks source link

i18n: documentation and typing problems #1103

Open elenakrittik opened 1 year ago

elenakrittik commented 1 year ago

The Problem

Currently, the LocalizationProtocol docs state that .get() must return dict[str, str] | None, implying that "if specific language is "supported", then it must support all keys used in slashcmds' names/descriptions/etc." (i'll refer to this as "the rule"). That is true to some degree: if we look at the code here, all APIs are type hinted as if they were expecting the rule to be followed, however, looking at the actual behavior and even error messages, it becomes clear that in fact disnake does handle keys that are translated only to certain languages (speaking strictly it just ignores such situations). It becomes even more confusing due to strict_localization controlling only key-level fails, not locale-level ones.

Suggested Solution

Change LocalizationProtocol.get() to return dict[str, str | None] | None. This would resolve the discrepancy between what's documented and what's implemented, while keeping backwards compatibility. Strict mode should also handle misses on any level (technically backwards-compatible too since so far type hints were abiding by the rule, however in practice i imagine this change can have larger impact).

Checklist

Additional Context

I am working on a LocalizationProtocol implementation, therefore this is more of an issue to those who implement custom localizers, not users of the default LocalizationStore.

Related #1104

I am willing to submit a PR for this.

shiftinv commented 1 year ago

For the most part this seems like a documentation issue. I don't think the type hints can be adjusted here, since the API is somewhat picky.

implying that "if specific language is "supported", then it must support all keys used in slashcmds' names/descriptions/etc."

Yeah, I agree it's a bit unclear. A language doesn't have to support every key; if it doesn't, then that language should just be left out of the dict returned by .get() for that key. For instance, .get("KEY_1") can return {"de": "...", "fr": "..."}, while .get("KEY_2") may just return {"de": "..."} if there's no french localization for KEY_2.

Changing this from dict[str, str] to dict[str, str | None] would require additional processing, since the API will complain if you provide null values in the localization dict (spec).

The short-term solution would be to clarify the documentation for the .get() return type. Long-term (i.e. v3 due to breaking changes), I'm hoping to refactor localizations since the current implementation is limited by a few factors, automatically resolving both this and #1104 in the process.

elenakrittik commented 1 year ago

As far as i can read the current implementation, changing the type hint to the proposed one will have 0 effect on any existing codebase, since it already handles Nones (by leaving them out, as you suggested).