comparison for regExp that compares pattern and modifiers #234

Merged
merged 3 commits into from Oct 27, 2012

Conversation

Projects
None yet
4 participants
@yopefonic
Contributor

yopefonic commented Jun 3, 2012

... objects were not properly compared resulting in non-matching RegExp objects to always return true. a patch to jasmine.Env.equals_ adds an extra step for RexExp objects to be compared.

related to issue pivotal/jasmine#199

@slevithan

This comment has been minimized.

Show comment Hide comment
@slevithan

slevithan Jul 17, 2012

+1 on letting toEqual compare RegExp objects. I need this in my tests, to ensure that regex copies are separate but equivalent instances (i.e., both not.toBe and toEqual comparisons should pass). A couple things you should add to the pull request, though:

  • Add comparison of the sticky property of the two regexes. This is supported in Firefox 3+, and will be standardized in ES6. In environments that don't support flag /y, the sticky property will simply be undefined.
  • Add a spec that compares separate regexes with the same pattern and flags but different values for their lastIndex properties. The two regexes should still pass the toEqual test.

Ref: my related QUnit issue.

+1 on letting toEqual compare RegExp objects. I need this in my tests, to ensure that regex copies are separate but equivalent instances (i.e., both not.toBe and toEqual comparisons should pass). A couple things you should add to the pull request, though:

  • Add comparison of the sticky property of the two regexes. This is supported in Firefox 3+, and will be standardized in ES6. In environments that don't support flag /y, the sticky property will simply be undefined.
  • Add a spec that compares separate regexes with the same pattern and flags but different values for their lastIndex properties. The two regexes should still pass the toEqual test.

Ref: my related QUnit issue.

@yopefonic

This comment has been minimized.

Show comment Hide comment
@yopefonic

yopefonic Jul 17, 2012

Contributor

I'll work on this either later tonight or tomorrow. Note that the pull requests on this change has been open for a while now so I don't know if it will be accepted by the maintainers at this time.

The /y flag documentation I found at MDN (https://developer.mozilla.org/en/JavaScript/Reference/Global_Objects/RegExp/).

What about the /u flag mentioned in the QUnit issue? I could not find any good documentation on it.

Contributor

yopefonic commented Jul 17, 2012

I'll work on this either later tonight or tomorrow. Note that the pull requests on this change has been open for a while now so I don't know if it will be accepted by the maintainers at this time.

The /y flag documentation I found at MDN (https://developer.mozilla.org/en/JavaScript/Reference/Global_Objects/RegExp/).

What about the /u flag mentioned in the QUnit issue? I could not find any good documentation on it.

@slevithan

This comment has been minimized.

Show comment Hide comment
@slevithan

slevithan Jul 17, 2012

What about the /u flag mentioned in the QUnit issue? I could not find any good documentation on it.

The /u flag is an accepted proposal for ES6, although some of its details remain to be worked out. There is not yet any good documentation for it. I'd wait on that one until there is at least one implementation. On the other hand, /y has been supported for years in Firefox.

What about the /u flag mentioned in the QUnit issue? I could not find any good documentation on it.

The /u flag is an accepted proposal for ES6, although some of its details remain to be worked out. There is not yet any good documentation for it. I'd wait on that one until there is at least one implementation. On the other hand, /y has been supported for years in Firefox.

@yopefonic

This comment has been minimized.

Show comment Hide comment
@yopefonic

yopefonic Jul 17, 2012

Contributor

I took the general standard while working on this and I wasn't aware of this Firefox specific implementation. +1 for pointing it out.

Contributor

yopefonic commented Jul 17, 2012

I took the general standard while working on this and I wasn't aware of this Firefox specific implementation. +1 for pointing it out.

@yopefonic

This comment has been minimized.

Show comment Hide comment
@yopefonic

yopefonic Jul 18, 2012

Contributor

work is done, note that I had to do some work around in the tests as you cannot create a regExp with a sticky flag outside of the browsers that support it. coverage is still okay in all browsers as the block is still called in the other tests as well. Only the block content can only be tested in browsers that do support this feature.

If there is any work that needs to be done to get this accepted please tell me.

Contributor

yopefonic commented Jul 18, 2012

work is done, note that I had to do some work around in the tests as you cannot create a regExp with a sticky flag outside of the browsers that support it. coverage is still okay in all browsers as the block is still called in the other tests as well. Only the block content can only be tested in browsers that do support this feature.

If there is any work that needs to be done to get this accepted please tell me.

@yopefonic

This comment has been minimized.

Show comment Hide comment
@yopefonic

yopefonic Jul 21, 2012

Contributor

updated the commit with a better test for the regExp with a sticky parameter as it failed in IE.

Contributor

yopefonic commented Jul 21, 2012

updated the commit with a better test for the regExp with a sticky parameter as it failed in IE.

@infews

This comment has been minimized.

Show comment Hide comment
@infews

infews Oct 26, 2012

Contributor

Can you please rebase this on top of HEAD and update? Thanks for the help!

Contributor

infews commented Oct 26, 2012

Can you please rebase this on top of HEAD and update? Thanks for the help!

@yopefonic

This comment has been minimized.

Show comment Hide comment
@yopefonic

yopefonic Oct 27, 2012

Contributor

will do

Contributor

yopefonic commented Oct 27, 2012

will do

yopefonic added some commits Jun 3, 2012

resolving issue that was identified via jasmine/jasmine#199 where Reg…
…Exp objects were not properly compared resulting in non-matching RegExp objects to always return true. a patch to jasmine.Env.equals_ adds an extra step for RexExp objects to be compared.
adding a check for the sticky regExp option supported by Firefox and …
…accepted by the ES6. Note that the tests for this case are checking for the support of the sticky parameter. the logic is still tested by the other expect statements in browsers that do not support sticky but will never enter that block as creating a regExp with that flag is not allowed. Coverage is still good. See jasmine/jasmine#234
@yopefonic

This comment has been minimized.

Show comment Hide comment
@yopefonic

yopefonic Oct 27, 2012

Contributor

@infews done

Contributor

yopefonic commented Oct 27, 2012

@infews done

ragaskar added a commit that referenced this pull request Oct 27, 2012

Merge pull request #234 from yopefonic/master
comparison for regExp that compares pattern and modifiers

@ragaskar ragaskar merged commit dbcb0b7 into jasmine:master Oct 27, 2012

@ragaskar

This comment has been minimized.

Show comment Hide comment
@ragaskar

ragaskar Oct 27, 2012

Contributor

Merged. Thanks. Would've preferred a single unified commit but forgot to ask for it. Next time. ;)

Thanks for the help!

Contributor

ragaskar commented Oct 27, 2012

Merged. Thanks. Would've preferred a single unified commit but forgot to ask for it. Next time. ;)

Thanks for the help!

@yopefonic

This comment has been minimized.

Show comment Hide comment
@yopefonic

yopefonic Oct 27, 2012

Contributor

I'll keep that in mind for next time. I'll do a squash before I do the pull request.

Contributor

yopefonic commented Oct 27, 2012

I'll keep that in mind for next time. I'll do a squash before I do the pull request.

ragaskar pushed a commit that referenced this pull request Dec 3, 2012

adding a check for the sticky regExp option supported by Firefox and …
…accepted by the ES6. Note that the tests for this case are checking for the support of the sticky parameter. the logic is still tested by the other expect statements in browsers that do not support sticky but will never enter that block as creating a regExp with that flag is not allowed. Coverage is still good. See jasmine/jasmine#234
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment