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

Feature jit locale #32

Merged
merged 6 commits into from
Jun 10, 2015
Merged

Feature jit locale #32

merged 6 commits into from
Jun 10, 2015

Conversation

grawk
Copy link
Member

@grawk grawk commented Jun 4, 2015

Adding locale support and unit tests

"type": "css",
"locator": ".myLocator"
},
"DE": {

Choose a reason for hiding this comment

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

I'd suggest using CLDR tags for locales, not country codes: try de-DE here.

@aredridel
Copy link

Really like this in concept, but we should be careful calling things a 'locale' that aren't. There's enough confusion out there already.

@grawk
Copy link
Member Author

grawk commented Jun 8, 2015

I totally agree. Do you have any suggestions for naming this feature?

@aredridel
Copy link

I'd suggest 'locale', at least make all the examples use BCP47 strings

@aredridel
Copy link

You could do bcp47 parse and match. That'd be interesting. So if defined "de" and the user specifies "de-DE" it'd find it. But that's some work.

@grawk
Copy link
Member Author

grawk commented Jun 8, 2015

This feature makes locators 2-dimensional as opposed to being specifically about locale. It wouldn't take into account the two separate portions of a true locale identifier. So I agree that "locale" is a bad name. It maybe is more like "specialization". But in asking for the feature, the term always used is "locale". I've not yet encountered anyone saying they need support for true locales, so I'm more in favor (at this point) of renaming things as opposed to providing something not yet asked for.

@aredridel
Copy link

Hmm. "overlay"? "specialization" is not a bad word

var locreator = this;
var locator = function () {
return locreator.normify(locreator.locatex(locatorJSON));
};
Copy link

Choose a reason for hiding this comment

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

Consider using _.once (https://lodash.com/docs#once) to cache the results of this initialization.
(Unless you expect to change locales more than once during the lifetime of the application)

Copy link
Member Author

Choose a reason for hiding this comment

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

Unfortunately that is exactly what this PR is about. The locale will change during the test suite lifecycle in a data driven scenario.

Choose a reason for hiding this comment

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

Also never add caching until you know you need it ;-)

V8 is fast.

Copy link
Member Author

Choose a reason for hiding this comment

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

Just to provide some more clarity here. "Locale" (really specialization) would change on the fly in a data-driven scenario. Currently the plugin lifecycle is tied to the webdriver lifecycle and the dynamically generated methods create a closure around their particular locator. So, unless the dynamic methods themselves are recreated when “locale” changes (which would be a significant refactor) this would be the only way to reflect the change to the locator.

The proposal in #33 would provide an avenue for recreating the dynamic methods when specialization changes, if we deem that there is a significant impact to performance to generate the locator on the fly.

@grawk
Copy link
Member Author

grawk commented Jun 10, 2015

Because this is a legacy feature, it cannot be renamed without breaking compatibility. As such I've created #33 to capture this discussion and to think through the correct way to rectify the issue. In the meantime I'd like to merge this, since it doesn't introduce the flawed nomenclature but simply makes the feature dynamic within the webdriver lifecycle.

@nikulkarni
Copy link
Contributor

👍

nikulkarni added a commit that referenced this pull request Jun 10, 2015
@nikulkarni nikulkarni merged commit 6e0aff0 into paypal:develop Jun 10, 2015
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

Successfully merging this pull request may close these issues.

4 participants