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

Forbid '=' as a cmp_ok operator #141

Closed
dolmen opened this issue Apr 29, 2011 · 4 comments
Closed

Forbid '=' as a cmp_ok operator #141

dolmen opened this issue Apr 29, 2011 · 4 comments

Comments

@dolmen
Copy link
Contributor

dolmen commented Apr 29, 2011

Using '=' as an operator is always a user mistake, as he probably wants '==' instead.
This mistake is currently not visible if $got is an lvalue.

Here is an example where Test::More should die in first test:

use Test::More tests => 1;
my @arr;
cmp_ok(scalar @arr, '=', 3);
cmp_ok(scalar @arr, '==', 0);

@dolmen
Copy link
Contributor Author

dolmen commented Apr 29, 2011

All other assignment operators should be forbiden too: += .= x= ^= |= ||= &&= ...

@schwern
Copy link
Contributor

schwern commented Aug 13, 2011

I concur. Assignment operators don't make much sense.

I don't know how far to take this. '=' vs '==' is an obvious trap. The rest not so much. I'd prefer a blacklist over a whitelist, as that allows people to get creative with comparison ops we haven't thought of (or get added like smartmatch).

wolfsage added a commit to wolfsage/test-more that referenced this issue Apr 1, 2012
@wolfsage
Copy link
Contributor

wolfsage commented Apr 1, 2012

#278

schwern pushed a commit that referenced this issue Apr 9, 2012
Signed-off-by: Michael G. Schwern <schwern@pobox.com>
schwern added a commit that referenced this issue Apr 9, 2012
Fix the test to properly predict the x= error from cmp_ok() based on our
passing in an error message rather than try the operator.

For #141
schwern added a commit that referenced this issue Apr 9, 2012
Signed-off-by: Michael G. Schwern <schwern@pobox.com>

For #141
schwern added a commit that referenced this issue Apr 9, 2012
@schwern
Copy link
Contributor

schwern commented Apr 9, 2012

Merged into the master branch, thanks!

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

No branches or pull requests

3 participants