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

lang.isArrayLike should return bool for falsey arguments and various other is* #106

Closed
wants to merge 3 commits into from

Conversation

Projects
None yet
4 participants
@megawac
Copy link
Contributor

commented Aug 10, 2014

As pointed out by @davidchambers (https://github.com/CrossEye/ramda/issues/237#issuecomment-51707816)

Fixes:

dojo.isArrayLike("") // => ""
dojo.isArrayLike(0) // => 0

Ticket: https://bugs.dojotoolkit.org/ticket/18251

@wkeese

This comment has been minimized.

Copy link
Member

commented Aug 10, 2014

Seems reasonable and along the same lines as #103. If you fix isArrayLike() though shouldn't you also fix isArray()?

@megawac

This comment has been minimized.

Copy link
Contributor Author

commented Aug 10, 2014

Hows that look @wkeese

@wkeese

This comment has been minimized.

Copy link
Member

commented Aug 11, 2014

I guess that's good. Not sure why we didn't do it that way that way to begin with, but reading through http://stackoverflow.com/questions/767486/how-do-you-check-if-a-variable-is-an-array-in-javascript it seems reasonable.

If this really works cross-frame as you say, on IE6+, then the API comment should be changed to remove the line about it now working.

@megawac

This comment has been minimized.

Copy link
Contributor Author

commented Aug 11, 2014

Yeah, youre right... I pieced this together rather quickly and punted on the cross frame tests as theyre a bit of a pain

@wkeese

This comment has been minimized.

Copy link
Member

commented Aug 20, 2014

OK, but we should still remove the comment. Other than that I think this is ready to be pushed. We talked about it at the meeting last week and there were no objections.

@dylans

This comment has been minimized.

Copy link
Member

commented Aug 20, 2014

👍

@megawac megawac force-pushed the megawac:arraylike-falsey branch 2 times, most recently from ee5697d to 70bfb98 Aug 20, 2014

@megawac megawac changed the title lang.isArrayLike should return bool for falsey arguments lang.isArrayLike should return bool for falsey arguments and various other is* Aug 20, 2014

@megawac

This comment has been minimized.

Copy link
Contributor Author

commented Aug 20, 2014

I updated this pr. I also made slight simplifications to isObject and isFunction while I was at it.

@megawac megawac force-pushed the megawac:arraylike-falsey branch from cf3229a to 70bfb98 Aug 20, 2014

@wkeese

This comment has been minimized.

Copy link
Member

commented Aug 21, 2014

I just tried this patch (all the commits) on FF31/Win7 and get these test errors:

     _AssertFailure: http://192.168.1.4/trunk/dojo/dojo.js line 345 > Function line 1 > eval:47 assertFalse('[object Window]') failed
     ERROR IN:
  (function isArray(t){ t.assertTrue(lang.isArray([])); t.assertTrue(lang.isArray(new Array())); t.assertFalse(lang.isArray({})); t.assertFalse(lang.isArray('')); t.assertFalse(lang.isArray(0)); t.assertFalse(lang.isArray(NaN)); t.assertFalse(lang.isArray(null)); t.assertFalse(lang.isArray(undefined)); t.assertFalse(window); t.assertFalse(Function); function Tricky() {} Tricky.prototype = []; t.assertFalse(lang.isArray(new Tricky)); })
 FAILED test: isArray 30 ms
     _AssertFailure: http://192.168.1.4/trunk/dojo/dojo.js line 345 > Function line 1 > eval:47 assertFalse('[object Window]') failed
     ERROR IN:
  (function isArrayLike(t){ t.assertFalse(lang.isArrayLike("thinger")); t.assertTrue(lang.isArrayLike(new Array())); t.assertFalse(lang.isArrayLike({})); t.assertTrue(lang.isArrayLike(arguments)); t.assertFalse(lang.isArrayLike("")); t.assertFalse(lang.isArrayLike(false)); t.assertFalse(lang.isArrayLike(NaN)); t.assertFalse(lang.isArrayLike(undefined)); t.assertFalse(lang.isArrayLike(null)); t.assertFalse(window); t.assertFalse(Function); t.assertTrue(lang.isArrayLike({0: 1, 1: 2, length: 2})); function Tricky() {} Tricky.prototype = []; t.assertTrue(lang.isArrayLike(new Tricky)); })
 FAILED test: isArrayLike 0 ms

Did I miss something?

@megawac

This comment has been minimized.

Copy link
Contributor Author

commented Aug 21, 2014

I was having trouble running the tests so I skipped them when I submitted this. I spent a couple minutes figuring them out and I'll look into it

t.assertFalse(lang.isArray(NaN));
t.assertFalse(lang.isArray(null));
t.assertFalse(lang.isArray(undefined));
t.assertFalse(window);

This comment has been minimized.

Copy link
@megawac

megawac Aug 21, 2014

Author Contributor

The failed test is this (and the following). Currently this returns true. Should it return true or false (noting length refers to frames)

Have fixes local: http://i.imgur.com/duKCOhe.png

This comment has been minimized.

Copy link
@wkeese

wkeese Aug 21, 2014

Member

This line and the next one aren't testing lang at all, why are they here?

This comment has been minimized.

Copy link
@megawac

megawac Aug 21, 2014

Author Contributor

They were typos. It's meant to be t.assertFalse(lang.isArray(window)) hence why the test failed

This comment has been minimized.

Copy link
@wkeese

wkeese Aug 21, 2014

Member

OK, and likewise for the tests below. So just add a new commit fixing those typos (and that passes regression) and I think we are good to go.

This comment has been minimized.

Copy link
@megawac

megawac Aug 21, 2014

Author Contributor

Makes sense, thanks. Amended that commit

@megawac megawac force-pushed the megawac:arraylike-falsey branch 3 times, most recently from fe96f55 to 9dd8b83 Aug 21, 2014

@megawac megawac force-pushed the megawac:arraylike-falsey branch from 9dd8b83 to 0bd4bb5 Aug 21, 2014

@wkeese wkeese self-assigned this Sep 3, 2014

@wkeese

This comment has been minimized.

Copy link

commented on 0bd4bb5 Sep 8, 2014

This is wrong, it will still fail on node because window is not defined (just try typing window in the node console, and see it throw an exception). The correct check is typeof window != "undefined". I will make that change and check this PR in.

@wkeese

This comment has been minimized.

Copy link
Member

commented Sep 8, 2014

OK, pushed in 4c89d55, thanks for the patches.

@wkeese wkeese closed this Sep 8, 2014

@wkeese

This comment has been minimized.

Copy link
Member

commented Sep 12, 2014

Woops, actually, with this patch now dijit/tests/form/test_Select.html?mode=test has a new failure on the FF 31 (Win7), and also FF 32 (Win7) related to the code in dojo/dijit@ad391fd:

_AssertFailure: http://192.168.1.4/trunk/dojo/dojo.js line 345 > Function line 1 > eval:47 assertEqual() failed:
    expected
        7
    but got
        14

" dojo.js line 345 > Function line 1 > eval:394

"   ERROR IN:
         (function test_setStore(t){
                            t.is(7, ds12.getOptions().length);
                            t.is("AL", ds12.value);
                            ds12.setStore(memoryStore2, "FL");
                            t.is(7, ds12.getOptions().length);
                            t.is("FL", ds12.value);
                            ds12.setStore(memoryStore, "CA");
                            t.is(7, ds12.getOptions().length);
                            t.is("CA", ds12.value);
                            ds12.setStore(memoryStore2);
                            t.is(7, ds12.getOptions().length);
                            t.is("DE", ds12.value);
                        })" dojo.js line 345 > Function line 1 > eval:400
"FAILED test: test_setStore 2 ms" dojo.js line 345 > Function line 1 > eval:400
"Total time for GROUP " dojo/store " is  117ms"

I guess it should be using isArrayLike() rather than isArray(). I'll change the code to do that.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
You can’t perform that action at this time.