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

better i18n Accept-Language parsing to properly support examples (+ as standalone function) #1850

Merged
merged 3 commits into from Jul 18, 2017

Conversation

Projects
None yet
3 participants
@WebFreak001
Contributor

WebFreak001 commented Jul 18, 2017

there is now a function to replicate the default behaviour of a translation context (as returning null simply uses the first element in languages) + the cases that were advertised in the examples (like rewriting en_GB to en_US if only that is available) actually work now and are unittested.
Validation for checking if there are ambigious languages in the allowed languages is still not implemented, but documented how it should be done to avoid issues.

I couldn't find the examples (http://vibed.org/docs#web-localization) in the repository so I didn't change the documentation there that now you can also use en-US or simply en (generic international fallback english) as language tags, but I added it to the translationContext documentation.

WebFreak001 added some commits Jul 18, 2017

More flexible standalone parsing of Accept-Language
there is now a function to replicate the default behaviour of a translation context (as returning null simply uses the first element in languages) + the cases that were advertised in the examples (like rewriting en_GB to en_US if only that is available) actually work now and are unittested.
Validation for checking if there are ambigious languages in the allowed languages is still not implemented, but documented how it should be done to avoid issues.
@s-ludwig

This is definitely good to have as a public API. Apart from the behavior change with regards to the fallback behavior this looks good.

Show outdated Hide outdated web/vibe/web/i18n.d
/// ditto
public string determineLanguageByHeader(Tuple...)(HTTPServerRequest req, Tuple allowed_languages) @safe pure
{
return determineLanguageByHeader(req.headers.get("Accept-Language", null), [allowed_languages]);

This comment has been minimized.

@s-ludwig

s-ludwig Jul 18, 2017

Member

also only(...)

@s-ludwig

s-ludwig Jul 18, 2017

Member

also only(...)

if (lcode == alang && !lextra.length)
return lang;
// request en* == serve en_* && be first occurence
if (lcode == alang && lextra.length && !fallback.length)

This comment has been minimized.

@s-ludwig

s-ludwig Jul 18, 2017

Member

There is one fallback case missing here: determineLanguageByHeader("en_US,enCA", ["en_GB"]) == "en_GB"). This should ideally still apply, though: determineLanguageByHeader("en_US,enCA", ["en_GB", "en"]) == "en").

@s-ludwig

s-ludwig Jul 18, 2017

Member

There is one fallback case missing here: determineLanguageByHeader("en_US,enCA", ["en_GB"]) == "en_GB"). This should ideally still apply, though: determineLanguageByHeader("en_US,enCA", ["en_GB", "en"]) == "en").

This comment has been minimized.

@WebFreak001

WebFreak001 Jul 18, 2017

Contributor

those tests already work, assuming the allowed_languages argument is valid (most generic languages should be to the right of the specific ones)

@WebFreak001

WebFreak001 Jul 18, 2017

Contributor

those tests already work, assuming the allowed_languages argument is valid (most generic languages should be to the right of the specific ones)

This comment has been minimized.

@s-ludwig

s-ludwig Jul 18, 2017

Member

Nevermind, I misread this as !lextra.length and it's actually fine like it is.

@s-ludwig

s-ludwig Jul 18, 2017

Member

Nevermind, I misread this as !lextra.length and it's actually fine like it is.

Show outdated Hide outdated web/vibe/web/i18n.d
/// ditto
public string determineLanguageByHeader(Tuple...)(string accept_language, Tuple allowed_languages) @safe pure
{
return determineLanguageByHeader(accept_language, [allowed_languages]);

This comment has been minimized.

@s-ludwig

s-ludwig Jul 18, 2017

Member

[allowed_languages] -> std.range.only(allowed_languages) to avoid GC allocations.

@s-ludwig

s-ludwig Jul 18, 2017

Member

[allowed_languages] -> std.range.only(allowed_languages) to avoid GC allocations.

@WebFreak001

This comment has been minimized.

Show comment
Hide comment
@WebFreak001

WebFreak001 Jul 18, 2017

Contributor

I only changed the behaviour of the documented unittest (example on the website) to show that you should do this instead of returning null because it gives the user the best user experience if the programmer overrides the provider to support cookies, etc. Before it would default to the first language in the array (en_US) if if wasn't in the session which would mean that potential matches of the Accept-Language header were lost.

Contributor

WebFreak001 commented Jul 18, 2017

I only changed the behaviour of the documented unittest (example on the website) to show that you should do this instead of returning null because it gives the user the best user experience if the programmer overrides the provider to support cookies, etc. Before it would default to the first language in the array (en_US) if if wasn't in the session which would mean that potential matches of the Accept-Language header were lost.

@s-ludwig s-ludwig added the auto-merge label Jul 18, 2017

@dlang-bot dlang-bot merged commit cecad1e into vibe-d:master Jul 18, 2017

2 checks passed

continuous-integration/appveyor/pr AppVeyor build succeeded
Details
continuous-integration/travis-ci/pr The Travis CI build passed
Details
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment