Comments

I'm assuming the possibility of a slightly different set of results won't be enough of a change to default behaviour to hold this back? If it gets the nod I'll commit the change.

Posted by Darby Felton (darby) on 2008-01-11T15:07:11.000+0000

bq. If it gets the nod I'll commit the change.

Can we see a patch and/or a summary of change impact? Thanks, Kevin, for looking at this! :)

Posted by Simone Carletti (weppos) on 2008-01-11T16:26:55.000+0000

{quote}I just tried this and it still passes unit tests.{quote}

As far I remember, there is no available unit test focused on this feature (and I suppose geo localization of responses is an information we cannot test via unit tests until a web service returns request parameters along with XML response). :)

I'm actually working with other web services provided by Yahoo! or companies that belongs to Yahoo! (such as Overture) and I noticed they are starting to implement a kind of geolocalization.

Providing a default 'en' language not only doesn't fit Yahoo! API requirements but may causes responses to be focused on a "default" market that is not the one Yahoo! would return in case the value is empty.

Providing a default value that is not a real default value (according to official documentation) is something I suggest to avoid.
This is why I suggested to remove this parameter from the array holding default options. :)

Posted by Kevin Golding (caomhin) on 2008-01-12T06:04:16.000+0000

Patch attached.

The change probably won't be very noticeable for anyone searching for english words like "sausage" or "New York" however it will be more obvious on searches that span many languages etc. Something like "Martina Hingis" will be more noticeable since she's Swiss and will have a fair number of results in assorted languages.

News is also more affected than Web since news searches are more likely to be topical - however as the results for those searches are far more volatile anyway the impact will be fairly short lived.

The unit tests that existed were pretty basic - they just set lang to 'Oops' and expect the validation to throw an exception which it does. Relying on exceptions from Yahoo is unfortunately random as they aren't consistent even with variables across different services (adult_ok for example).

I'm with Simone about the bogus default and I think the impact is negligible so I think the change is valid.

Posted by Simone Carletti (weppos) on 2008-01-15T02:35:26.000+0000

Hi Kevin,
I gave a look at the patch and it seems to be the most reasonable thing to do.

We probably need to add some unit tests in order to ensure lang options is null by default and validation works if a language value is supplied.