Skip to content
This repository was archived by the owner on Jul 29, 2024. It is now read-only.

feat(locators): Add By.exactBinding locator #812

Merged
merged 1 commit into from
May 17, 2014

Conversation

hankduan
Copy link
Contributor

No description provided.

@juliemr
Copy link
Member

juliemr commented May 16, 2014

LGTM!

@hankduan hankduan added cla: yes and removed cla: no labels May 16, 2014
hankduan added a commit that referenced this pull request May 17, 2014
feat(locators): Add By.exactBinding locator
@hankduan hankduan merged commit 98f4ba5 into angular:master May 17, 2014
matches.push(bindings[i]);
if (exactMatch) {
var matcher = new RegExp('([^a-zA-Z\d]|$)' + binding + '([^a-zA-Z\d]|^)');
if (matcher.test(bindingName)) {

Choose a reason for hiding this comment

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

There seems to be an issue regarding the regex. When running npm testI get the error:
lib/clientsidescripts.js: line 54, col 45, Bad or unnecessary escaping.

I think it should be changed to:
new RegExp('([^a-zA-Z\\d]|$)' + binding + '([^a-zA-Z\\d]|^)');

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Thanks for letting me know. Currently the code works fine with the Regex object, but fails jshint. I'll make that change.

@hankduan
Copy link
Contributor Author

@edconolly Do you have a test case where the regex doesn't capture a brace binding name i.e.{{myBinding}}? It's been working for the tests I've ran, so if you can give me a counterexample, it would be much appreciated.

Using 'bindingName.replace(/{|}||.*/g, '') === binding' would not cover cases like
'{{ binding }}' -- extra space, '({{nickname|uppercase}})' -- parenthesis, 'Fruit: {{fruit}}', '{{check.w}}{{check.x}}{{check.y}}{{check.z}}', etc. It feels to me that it is easier to say what characters are allowed instead of remove those which are not.

@edconolly
Copy link

Apologies I provided an out of date version from which I had locally. This statement is better:

.replace(/{|}|\s||.*/g, '')

although the Fruit / list example (although I’m not sure if bindings get returned like that) would still not match

A test case where the regex match fails is for {{myBinding.notThisVar}}, the regex will match '{myBinding.’ and return that value. Either way I think it would be good to pad out the tests to cover some of these edge cases.

From: Hank Duan <[email protected]mailto:[email protected]>
Reply-To: angular/protractor <[email protected]mailto:[email protected]>
Date: Monday, 19 May 2014 18:29
To: angular/protractor <[email protected]mailto:[email protected]>
Cc: Ed Conolly <[email protected]mailto:[email protected]>
Subject: Re: [protractor] feat(locators): Add By.exactBinding locator (#812)

@edconollyhttps://github.com/edconolly Do you have a test case where the regex doesn't capture a brace binding name i.e.{{myBinding}}? It's been working for the tests I've ran, so if you can give me a counterexample, it would be much appreciated.

Using 'bindingName.replace(/{|}||.*/g, '') === binding' would not cover cases like
'{{ binding }}' -- extra space, '({{nickname|uppercase}})' -- parenthesis, 'Fruit: {{fruit}}', '{{check.w}}{{check.x}}{{check.y}}{{check.z}}', etc. It feels to me that it is easier to say what characters are allowed instead of remove those which are not.


Reply to this email directly or view it on GitHubhttps://github.com//pull/812#issuecomment-43532672.

WDFC UK Limited. Registered in England & Wales with registered number 6374235 and registered office 88 Crawford Street, London W1H 2EJ. Authorised and regulated by the Financial Conduct Authority. Interim Permission Number 611974. Any communication sent by or on behalf of WDFC UK Limited or any of its subsidiary, holding or affiliated companies or entities (together "Wonga") is confidential and may be privileged or otherwise protected. If you receive it in error please inform us and then delete it from your system. You should not copy it or disclose its contents to anyone. Messages sent to and from Wonga may be monitored to ensure compliance with our internal policies and to protect our business. Emails are not secure and cannot be guaranteed to be error free. Anyone who communicates with us by email is taken to accept these risks.

This email has been scanned for email related threats and delivered safely by Mimecast.

For more information please visit http://www.mimecast.com

@hankduan
Copy link
Contributor Author

@edconolly it was a conscience decision to match myBinding in {{myBinding.notThisVar}}. This way you can match either myBinding or myBinding.notThisVar (but not partials of those).
With regards to those examples, they're taken directly from protractor's test app, so they're definitely valid binding representations from compiled angular.

@afternoon
Copy link

How is exactBinding different from binding here? If I say exactBinding in a test, I expect it to be bound to that exact variable, not some other thing.

For example, with the following template:

Your total is {{total.amount}} (includes {{total.tax}} tax)

I want to ensure that both total.amount and total.tax are bound in my test. With your current implementation, the test would pass if only one of those were bound.

@hankduan
Copy link
Contributor Author

use by.exactBinding('total.amount') and by.exactBinding('total.tax') to find your binding.

If you have an example like this:

  • {{fooBar}}
  • {{foo}}

by.binding('foo') would match both, whereas by.exactBinding('foo') would only find the second binding

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

Successfully merging this pull request may close these issues.

5 participants