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

Describe localized text "default" alias #732

Open
wants to merge 1 commit into
base: master
Choose a base branch
from

Conversation

jonas-paul
Copy link
Contributor

Summary

Added a description for this change: https://github.com/MewsSystems/mews/pull/64699.

Checklist

  • Documentation follows the Style Guide
  • JSON examples updated
  • Properties in JSON examples are in the same order as in property tables
  • Changelog dated the day when PR merged
  • Changelog accurately describes all changes
  • Changelog highlights the affected endpoints or operations
  • Changelog highlights any deprecations
  • All hyperlinks tested
  • Deprecation Table updated if any deprecations
  • SUMMARY.md updated if new pages added

@jonas-paul jonas-paul self-assigned this Feb 6, 2025
@jonas-paul jonas-paul requested review from a team as code owners February 6, 2025 14:03
Copy link
Contributor

@MikeAdamsMews MikeAdamsMews left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@jonas-paul I don't understand.

@jonas-paul
Copy link
Contributor Author

jonas-paul commented Feb 6, 2025

@jonas-paul I don't understand.

@MikeAdamsMews it is about a special key default in localized string:

{
    "default": "Children"
}

It means this for an enterprise which has default language set to en-US:

{
    "en-US": "Children"
}

while it means this for an enterprise which has default language set to cs-CZ:

{
    "cs-CZ": "Children"
}

Is this clearer?

@jnv
Copy link
Member

jnv commented Feb 7, 2025

I'd suggest a bit more details and clarifying the conflict behavior. For example:

{
  "default": "Children",
  "de-DE": "Kinder",
  "cs-CZ": "Děti"
}

Assuming the enterprise's default language is en-UK, this will be evaluated as:

{
  "en-UK": "Children",
  "de-DE": "Kinder",
  "cs-CZ": "Děti"
}

In case the default language is already present in the dictionary, the explicit language takes precedence over the default key, for example:

{
  "default": "Value for default key",
  "en-US": "Value for explicit language key"
}

If the enterprise's default language is en-US, the default key will be ignored and the dictionary will be evaluated as:

{
  "en-US": "Value for explicit language key"
}

Also don't forget to update changelog.

@MikeAdamsMews
Copy link
Contributor

@jonas-paul sorry but this feels wrong to me, and no it's not clear. I'm actually stuck on the first statement "When used as an input parameter". We have no concept of an input parameter, I assume you mean when using a field of type localized text as part of a request, e.g. Add rates? Do you mean that in every instance in the API where localized text is used as part of a request, it can have this behaviour? It seems that you are describing a fairly complicated behaviour, when this should be nothing more than a data type declaration. localized text is already one of the more difficult data types to describe, because its contents are variable. I'm also curious as to why it's necessary, does the API client not know what the enterprise language is set to? In that case, surely it's not qualified to specify any name strings? I think to describe this properly we need a fuller description, including examples, but I am questioning why we have such behaviour linked to a data type in the first place.

@jonas-paul
Copy link
Contributor Author

@MikeAdamsMews you're correct on all the points. Localized text type is used both in requests and responses, so "input parameter" was an attempt to describe the request part. This feature is a workaround for integration with Choice. They are a big client and in some cases they are reluctant to use the API as intended by us so we have made some "compromises" along the way. This is one of them, it is a quick way to solve their problem and within the team we agreed that it is a good way to go. I agree that it doesn't go too well with how data types should be defined in the API, but it is a behavior applicable everywhere that type is used.

What can we do about this?

  1. Keep the behavior and make localized text description clearer.
  2. Keep the behavior, leave localized text description as it was, but mention this behavior next to every field it applies to.
  3. Keep the behavior, but don't document it.
  4. Drop the behavior and push Choice to fetch enterprise configuration to get the language code but still send English text (they don't have translations) with whatever code they get (this is a bit weird and, understandably, that's where their reluctance comes from).
  5. Find out why this requirement for having the name in default language exists and try to solve it in another way.

@jnv, @ondrahermanek what do you think?

@ondrahermanek
Copy link
Contributor

4 - keep it hidden, so we don't make this behavior "official"? As Jonas says, it is a workaround ...

@jnv
Copy link
Member

jnv commented Feb 7, 2025

Agree with Ondra, perhaps keep this behavior undocumented for now (so option 3.) and se how - and if - they use it.

@MikeAdamsMews
Copy link
Contributor

Thanks for the context @jonas-paul. I'm not keen on concepts like 'workaround', equally I'm not keen on hidden behaviours. From a documentation POV I'm just asking a few basic questions: is it technically correct? is it consistent with the rest of the API? is it clear how it works? and does it make sense? As long as you can answer those questions, I'm happy :-)

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

5 participants