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

Example for loose option is not correct #31

Open
jonasholtkamp opened this issue Jul 30, 2018 · 7 comments
Open

Example for loose option is not correct #31

jonasholtkamp opened this issue Jul 30, 2018 · 7 comments

Comments

@jonasholtkamp
Copy link

In your README it states that parser.pick(['fr', 'en'], 'en-GB,en-US;q=0.9,fr-CA;q=0.7,en;q=0.8'); would return "fr". But that's not correct IMO:

> parser.pick(['fr', 'en'], 'en-GB,en-US;q=0.9,fr-CA;q=0.7,en;q=0.8');
'en'
> parser.pick(['fr', 'en'], 'en-GB,en-US;q=0.9,fr-CA;q=0.7,en;q=0.8', { loose: true });
'en'

I'd say this is indeed the expected behaviour, because there are to en with q=1 which should have preference over the fr locale.

Bonus: The README's example for the loose option is missing the { loose: true }.

@bamapookie
Copy link

I added some tests to indicate how pick handles quality when loose is true in #35

@alex-piccione
Copy link

alex-piccione commented Jan 28, 2022

@bamapookie
I don't think this is right:

it('selects the first matching language in loose mode, even when the accepted language is more restrictive', function(){
        var result = parser.pick(['en', 'en-US', 'pl'], 'en-US;q=0.6', { loose: true });
        assert.equal(result, 'en');
    });

If "en-US" is accepted and "en-US" is supported it should return "en-US", why prefer a partial matching that is also less restrictive?
Imagine I'm in the UK; accepted language is "en-GB".
With these supported languages, ["en", "en-GB"], it's not correct return "en", which has date format MM/dd/yyyy.
"en-GB" is supported and I expect a date in the format dd/MM/yyyy.

@bamapookie
Copy link

@alex-piccione
From the readme:

In loose mode the order of supportedLanguagesArray matters, as it is the first partially matching language that is returned. It means that if you want to pick more specific langauges first, you should list it first as well.

I designed the test to match the requirement in this text.

@bamapookie
Copy link

I'll update the test if you want. Let me know the criteria you want, and I'll update the readme as well.

Example:
specific language/region match > generic language earlier in specificLanguagesArray, etc.

@alex-piccione
Copy link

alex-piccione commented Jan 28, 2022

In the previous comment I explained a specific example for a case where picking the first matching lang-region is not correct if a generic language exists (also if it came after in the accepted languages array).

  1. I believe if a 1 to 1 matching locale exists (language-region) it does not make sense pick the first loose matching (language only); that should be the choice in case of missing perfect match.
  2. More than that I suppose that in general (not only for "en") the generic language locale (xx) could be better than a specific language-region (xx-aa) if the region does not match with mine (xx-bb).
it('1. selects the first matching language in loose mode, even when the accepted language is more restrictive', function(){
    var result = parser.pick(['en', 'en-US', 'pl'], 'en-US', { loose: true });
    assert.equal(result, 'en-US');
});

it('2. select less restrictive but proper language when availabe also if not first match', function(){
    var result = parser.pick(['en-US', 'en'], 'en-GB', { loose: true });
    assert.equal(result, 'en');
});

I just found someone else already suggested point 1: #34 (comment)

@bamapookie
Copy link

bamapookie commented Jan 28, 2022

Those 2 tests currently fail. They would require changing how loose is evaluated in the parser. My intention was to demonstrate how the code actually functions (except for the one failing test that needs to be changed from equal to deepStrictEqual).

Edit: fixed and pushed.

@alex-piccione
Copy link

Perfect, I'll open an issue.

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

No branches or pull requests

3 participants