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

i18n: documentation and typing problems #1103

Open
3 tasks done
elenakrittik opened this issue Sep 3, 2023 · 2 comments
Open
3 tasks done

i18n: documentation and typing problems #1103

elenakrittik opened this issue Sep 3, 2023 · 2 comments
Labels
bug Something isn't working t: documentation Improvements or additions to documentation/examples

Comments

@elenakrittik
Copy link
Contributor

elenakrittik commented Sep 3, 2023

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

  • I have searched the open issues for duplicates.
  • I have shown the entire traceback, if possible.
  • 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 elenakrittik added the unconfirmed bug Something might not be working label Sep 3, 2023
@shiftinv shiftinv added bug Something isn't working t: documentation Improvements or additions to documentation/examples and removed unconfirmed bug Something might not be working labels Sep 8, 2023
@shiftinv
Copy link
Member

shiftinv commented Sep 8, 2023

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

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 elenakrittik changed the title i18n: Misleading documentation and type hints i18n: documentation and typing problems Apr 5, 2024
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
bug Something isn't working t: documentation Improvements or additions to documentation/examples
Projects
None yet
Development

No branches or pull requests

2 participants