disnake icon indicating copy to clipboard operation
disnake copied to clipboard

i18n: documentation and typing problems

Open elenakrittik opened this issue 2 years ago • 2 comments

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

  • [X] I have searched the open issues for duplicates.
  • [X] I have shown the entire traceback, if possible.
  • [X] I have removed my token from display, if visible.

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.

elenakrittik avatar Sep 03 '23 15:09 elenakrittik

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.

shiftinv avatar Sep 08 '23 13:09 shiftinv

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).

elenakrittik avatar Sep 08 '23 13:09 elenakrittik