Fix present validation check #43

Merged
merged 2 commits into from Mar 23, 2013

Projects

None yet

3 participants

@lgomezma
Contributor

Fixes the 'present' validation check as explained in Issue #42

@lgomezma
Contributor

I added the empty string check in a new pull request but not sure about how to write the tests...shame on me :P

I need to learn tests and github :)

@lgomezma lgomezma closed this Mar 22, 2013
@lgomezma lgomezma reopened this Mar 22, 2013
@ben-ng
Contributor
ben-ng commented Mar 22, 2013

Writing the test case now!

Here we go: https://gist.github.com/ben-ng/5223019

@mde
Contributor
mde commented Mar 22, 2013

Good call, we need to start being a little more diligent about including tests with our fixes. @ben-ng Can we get your test in a PR?

You guys are amazing. I can't keep up. :)

@ben-ng ben-ng pushed a commit to ben-ng/model that referenced this pull request Mar 22, 2013
Ben Ng Add tests for Issue #42 (PR #43) bd95f58
@ben-ng
Contributor
ben-ng commented Mar 22, 2013

Here you go: #45

@lgomezma
Contributor

Thanks Ben for the test! :)

@ben-ng
Contributor
ben-ng commented Mar 23, 2013

@mde is @igomezma 's fix good to go? this issue is a blocker for me.

@mde mde merged commit 199f82b into geddy:master Mar 23, 2013
@mde
Contributor
mde commented Mar 23, 2013

Thanks!

@mde
Contributor
mde commented Mar 23, 2013

Pushed to NPM in v0.0.39.

On Fri, Mar 22, 2013 at 11:05 PM, Ben notifications@github.com wrote:

@mde https://github.com/mde is @igomezma 's fix good to go? this issue
is a blocker for me.


Reply to this email directly or view it on GitHubhttps://github.com/mde/model/pull/43#issuecomment-15332429
.

@ben-ng
Contributor
ben-ng commented Mar 23, 2013

Thanks for fixing this on a Friday night!

On Mar 23, 2013, at 12:06 AM, Matthew Eernisse notifications@github.com
wrote:

Pushed to NPM in v0.0.39.

On Fri, Mar 22, 2013 at 11:05 PM, Ben notifications@github.com wrote:

@mde https://github.com/mde is @igomezma 's fix good to go? this issue
is a blocker for me.


Reply to this email directly or view it on GitHub<
https://github.com/mde/model/pull/43#issuecomment-15332429>
.


Reply to this email directly or view it on
GitHubhttps://github.com/mde/model/pull/43#issuecomment-15332935
.

@lgomezma
Contributor

Thanks to both of you :)

@ben-ng
Contributor
ben-ng commented Mar 23, 2013

Okay, I don't know if this is just me being stupid but -- the fix doesn't appear to be working. My submitted test case still fails on 0.0.39, and my app's unit tests are still broken on the latest geddy on npm too. Can someone else confirm this?

@ben-ng
Contributor
ben-ng commented Mar 23, 2013

Oh wait, i see the problem. The string comparison was done with a ==. I'll send a new PR.

@lgomezma
Contributor

In that case we need to check for the null case as well since this evaluates to false 'null===undefined' and therefore it will accept null values, which looking at the comment that mde wrote on the code should also fail.

@ben-ng
Contributor
ben-ng commented Mar 23, 2013

@lgomezma you're right. i'll add more tests and fix that.

@lgomezma
Contributor

just added a pull request to fix that :)

@mde
Contributor
mde commented Mar 23, 2013

Ah, fuck zero is double-equal to empty string. Goddammit. :)

@ben-ng
Contributor
ben-ng commented Mar 23, 2013

@mde it might be a good idea to start requiring jshint compliance to prevent stuff like this from happening

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