fluid-work IRC Logs-2012-07-31

Owned by Fluid IRC Log Bot
Last updated: Jul 31, 2012Version comment
11 min read

[11:20:36 CDT(-0500)] <michelled> anastasiac: did you see that Andrey pushed 660?

[11:20:57 CDT(-0500)] <anastasiac> thanks, michelled. I checked this morning, and it wasn't there yet. Nice to see it done

[11:24:20 CDT(-0500)] <anastasiac> cindyli, I don't know if you saw that I got a staff on the Facebook and twitter icons. I did the ones in the lower right side of the page (not the ones that are in an iframe). There's a link to my github branch on the iteration page

[11:25:04 CDT(-0500)] <cindyli> that's great, thanks for letting me know, anastasiac

[11:44:56 CDT(-0500)] <cindyli> anastasiac: the ticket for the issue we talked about - http://issues.fluidproject.org/browse/FLOE-57. The odd thing is now I don't see that comment section in IE any more, with login or logout.

[11:45:10 CDT(-0500)] <cindyli> with index or without

[13:05:47 CDT(-0500)] <michelled> anastasiac: I just looked at your FLOE-53 branch - it seems that line spacing changes sill causes issues for the My OER header

[13:06:09 CDT(-0500)] <anastasiac> ok, michelled, I'll have a look

[13:53:26 CDT(-0500)] <Justin_o> Bosmon: are you around.. had a question about that model merging issue i have

[13:53:40 CDT(-0500)] <anastasiac> michelled, I've pushed a fix for FLOE-53

[13:53:46 CDT(-0500)] <Bosmon> Hi Justin_o

[13:53:48 CDT(-0500)] <michelled> thx anastasiac

[13:53:56 CDT(-0500)] <Justin_o> Bosmon: hello..

[13:54:43 CDT(-0500)] <Justin_o> so i'm trying to write something up for it… but what i'm finding is that in mergeImpl the thisTarget is undefined..

[13:55:27 CDT(-0500)] <Bosmon> Justin_o - it may well be

[13:55:32 CDT(-0500)] <Bosmon> In the case there is no left-hand value

[13:55:54 CDT(-0500)] <Justin_o> i guess i should send you some code

[13:56:29 CDT(-0500)] <michelled> anastasiac, cindyli: a11y-uio has been updated with anastasiac's fixes (smile)

[13:56:44 CDT(-0500)] <Justin_o> Bosmon: this is from my unit tests http://pastebin.com/PDVaa6Uh

[13:57:38 CDT(-0500)] <Justin_o> Bosmon: the result of which is that the model is just an empty object {}

[13:58:03 CDT(-0500)] <Bosmon> Are you returning anything from your merge policy function?

[13:59:40 CDT(-0500)] <Justin_o> Bosmon: here's the implemented code

[13:59:41 CDT(-0500)] <Justin_o> http://pastebin.com/CDzcXJTk

[13:59:47 CDT(-0500)] <Justin_o> with the test at the bottom

[14:01:27 CDT(-0500)] <Bosmon> You are overusing "typeof" checks

[14:01:42 CDT(-0500)] <Justin_o> Bosmon: that's possible yes

[14:01:43 CDT(-0500)] <Bosmon> For the undefined checks you can just compare against the undefined value

[14:01:55 CDT(-0500)] <Bosmon> For the object check you run into the risk that typeof(null) === "object"

[14:02:08 CDT(-0500)] <Justin_o> Bosmon: forgot about those thanks

[14:03:49 CDT(-0500)] <Bosmon> You are currently falling through on the case that the model path's value is neither undefined nor an object

[14:03:54 CDT(-0500)] <Bosmon> For example, any primitive value

[14:05:06 CDT(-0500)] <Justin_o> i guess i need to return the source like in fluid.model.mergeModel

[14:05:21 CDT(-0500)] <Bosmon> Ultimately

[14:05:33 CDT(-0500)] <Bosmon> You can work out the fine points of object identity once you have at least the correct value returned

[14:06:49 CDT(-0500)] <Justin_o> Bosmon: okay.. well that last bit fixed it, but it could be that my test isn't quite good enough

[14:07:06 CDT(-0500)] <Bosmon> You will need a considerable array of tests

[14:07:16 CDT(-0500)] <Bosmon> For an issue as subtle as this one : P

[14:07:47 CDT(-0500)] <Justin_o> Bosmon: okay.. fixed my test a bit so it's breaking again (smile)

[14:08:17 CDT(-0500)] <Justin_o> Bosmon: the new and old models were too similar.. same keys.. so the straight replacement looked like it was merging..

[14:11:54 CDT(-0500)] <anastasiac> michelled, I've pushed a fix for FLOE-57 to my github: https://github.com/acheetham/OER-Commons/tree/FLOE-57

[14:12:22 CDT(-0500)] <anastasiac> michelled, note that to properly test in IE, you'll need to edit development.py, set COMPRESS = True, and restart your VM

[14:13:52 CDT(-0500)] <Justin_o> Bosmon: these are the test cases i currently have for the the merge function itself.. any glaring omission that you notice there? http://pastebin.com/9iX1FMzF

[14:14:26 CDT(-0500)] <Bosmon> Yes, the omission of writing these tests declaratively (smile)

[14:14:39 CDT(-0500)] <Justin_o> Bosmon: (smile)

[14:14:52 CDT(-0500)] <Justin_o> well they're just a start for now.. always room for refactoring

[14:15:24 CDT(-0500)] <Bosmon> Yes - but you will be aware that starting down a cut n paste route usually obliges you to carry on that way : P

[14:15:40 CDT(-0500)] <Justin_o> Bosmon: i can't argue with that

[14:16:04 CDT(-0500)] <Bosmon> In terms of your "isPrimitive" check - the cases you might need to consider might look something like ... , modelA, undefined, {}, modelB, ....

[14:16:13 CDT(-0500)] <Bosmon> That is, where this is the stack of arguments supplied to merge

[14:16:26 CDT(-0500)] <Bosmon> So there may well be several "gaps" in between the 2 models before they reach each other

[14:16:37 CDT(-0500)] <Bosmon> Your function needs to make sure that things "cascade to the left" properly

[14:17:06 CDT(-0500)] <Bosmon> So returning nothing in the case you don't like your left-hand argument will most likely not be a win

[14:17:47 CDT(-0500)] <Bosmon> That's just a guess at what is going on, without seeing the real merge stack in detail...

[14:18:58 CDT(-0500)] <Justin_o> Bosmon: okay.. fixing up some of those other things you mentioned now.. i'll let you see them soon.. then i'll try to test this out again

[14:19:02 CDT(-0500)] <Bosmon> cool

[14:33:06 CDT(-0500)] <michelled> anastasiac, cindyli: can we have a quick planning meeting?

[14:33:20 CDT(-0500)] <cindyli> sure, michelled

[14:49:48 CDT(-0500)] <Justin_o> Bosmon: is there any way to do something like this

[14:50:33 CDT(-0500)] <Justin_o> a1 = ["a"]; a2 = ["b"] f(a1, a2); a1 === a2

[14:50:37 CDT(-0500)] <Justin_o> if you get what i mean

[14:54:12 CDT(-0500)] <Bosmon> Justin_o - there isn't

[14:54:18 CDT(-0500)] <Bosmon> All values are passed by reference in JS

[14:54:22 CDT(-0500)] <Justin_o> Bosmon: (sad)

[14:54:41 CDT(-0500)] <Bosmon> You should be glad of it! : P

[14:54:45 CDT(-0500)] <Justin_o> Bosmon: i guess i could do my first example by doing a1[0] = a2[0]

[14:54:59 CDT(-0500)] <Bosmon> Do you mean your first test case?

[14:55:01 CDT(-0500)] <Justin_o> but i can't do the case where a2 is an ojbect

[14:55:10 CDT(-0500)] <Bosmon> Which is your first example

[14:55:21 CDT(-0500)] <Justin_o> where a1 and a2 are both arrays

[14:55:32 CDT(-0500)] <Bosmon> Can you be more specific?

[14:55:36 CDT(-0500)] <Justin_o> (smile)

[14:55:55 CDT(-0500)] <Justin_o> a1 = ["a"]; a2 = ["b"] f(a1, a2); a1 === a2

[14:56:08 CDT(-0500)] <Bosmon> Yes - why is that a thing you want to achieve?

[14:56:25 CDT(-0500)] <Justin_o> well my whole goal is that i want to be able to replace arrays

[14:56:30 CDT(-0500)] <Bosmon> Well, sure

[14:56:48 CDT(-0500)] <Bosmon> But you only want to replace them in the context of their being registered as part of the "model" section of an options block

[14:57:06 CDT(-0500)] <Justin_o> however if the top level thing is an array instead of an object the target won't be changed, like if it were an object.. although the return value will be correct

[14:57:27 CDT(-0500)] <Justin_o> Bosmon: yes.. so it is guaranteed to always be within an object?

[14:57:37 CDT(-0500)] <Bosmon> Well, you can't really test this outside the context of operating "fluid.merge"

[14:57:50 CDT(-0500)] <Bosmon> And yes, this could never be an effective root policy - since the root of every options block is guaranteed to be an object

[14:58:17 CDT(-0500)] <Justin_o> okay.. good.. so even if i had a default block that had model: []

[14:58:21 CDT(-0500)] <Justin_o> it would still be okay?

[14:58:25 CDT(-0500)] <Bosmon> You could test that, yes

[14:58:43 CDT(-0500)] <Bosmon> But that now gets you past your issue about worrying about "root object identity"

[14:58:47 CDT(-0500)] <Justin_o> okay.. i have a test for an array nested in an object and that passes fine

[14:58:55 CDT(-0500)] <Bosmon> Cool

[14:58:55 CDT(-0500)] <Justin_o> okay

[14:59:03 CDT(-0500)] <Justin_o> i'll just drop some tests (smile)

[14:59:34 CDT(-0500)] <Justin_o> and i might be able to refactor the code a bit now since i can always assume an object

[14:59:48 CDT(-0500)] <Bosmon> It shouldn't make any difference to you

[14:59:57 CDT(-0500)] <Bosmon> Since it is only fluid.merge that ever worries about the root object

[15:00:09 CDT(-0500)] <Bosmon> YOUR root object might well be an array - the thing held in model: []

[15:00:14 CDT(-0500)] <Justin_o> Bosmon: actually you're probably right.. i'll have to use that other part in my recursion anyways

[15:00:23 CDT(-0500)] <Justin_o> Bosmon: hmm

[15:00:39 CDT(-0500)] <Justin_o> Bosmon: so i do have to worry about getting an array passed directly

[15:00:41 CDT(-0500)] <Bosmon> but really, object identity is only really important for the model root

[15:00:53 CDT(-0500)] <Bosmon> Justin_o - to your merge function, yes

[15:00:55 CDT(-0500)] <Justin_o> do i actually need to worry about the target being changed, or does only the return value actually matter

[15:01:09 CDT(-0500)] <Justin_o> Bosmon: ^

[15:01:14 CDT(-0500)] <Bosmon> Both are potentially important

[15:01:29 CDT(-0500)] <Bosmon> But given you only ever dispatch back to your own policy function, only you might care about the identity of the target object

[15:01:47 CDT(-0500)] <Bosmon> The merge function on the outside just makes use of your return value

[15:01:49 CDT(-0500)] <Justin_o> Bosmon: so i do have a problem if the i get the target being an array.. since i can never hope to change this

[15:02:09 CDT(-0500)] <Bosmon> Well, you don't need to change it - you just need to return the right value

[15:02:16 CDT(-0500)] <Justin_o> Bosmon: okay..then i'm fine

[15:02:25 CDT(-0500)] <Bosmon> Look outside into mergeImpl to see how your return value is used

[15:02:43 CDT(-0500)] <Bosmon> if (funcPolicy) {

[15:02:43 CDT(-0500)] <Bosmon> target[name] = newPolicy.call(null, thisTarget, thisSource, name);

[15:02:49 CDT(-0500)] <Bosmon> This is what your dispatch site looks like

[15:03:16 CDT(-0500)] <Justin_o> Bosmon: okay.. good.. so the fact that $.extend actually changes the target doesn't really matter

[15:03:28 CDT(-0500)] <Justin_o> in this case at least

[15:03:49 CDT(-0500)] <Bosmon> In general the contract is that the merge operation corrupts all of its operands, yes : P

[15:04:32 CDT(-0500)] <Bosmon> It may not be the most helpful of contracts, but really we can't afford to duplicate the entire merge stack... since merging is probably the most CPU and memory-intensive activity in the framework

[15:05:16 CDT(-0500)] <Justin_o> Bosmon: okay.. i think i get it

[15:05:26 CDT(-0500)] <Justin_o> Bosmon: here's a rewrite of the tests.

[15:05:26 CDT(-0500)] <Justin_o> http://pastebin.com/tcAV2wx4

[15:05:40 CDT(-0500)] <Bosmon> Except for the very left-most object, you have to assume that anything you send in to fluid.merge is something you no longer care about

[15:06:02 CDT(-0500)] <Bosmon> Which implies duplicating things if necessary yourself

[15:06:42 CDT(-0500)] <Bosmon> Sorry, actually that's not quite right

[15:06:49 CDT(-0500)] <Bosmon> I guess I should have at least written what the contract is

[15:07:05 CDT(-0500)] <Bosmon> In fact, all the arguments are guaranteed to be preserved, EXCEPT for the leftmost one (smile)

[15:07:06 CDT(-0500)] <Justin_o> Bosmon: here's my new code http://pastebin.com/35HL3JQA

[15:07:16 CDT(-0500)] <Justin_o> Bosmon: ah yes.. the target

[15:07:26 CDT(-0500)] <Justin_o> Bosmon: any number of source arguments are preserved

[15:07:55 CDT(-0500)] <Justin_o> Bosmon: although does my function actually need to worry about multiple sources, didn't look like more than one was ever passed in

[15:08:04 CDT(-0500)] <Bosmon> Yes, the policy just deals with things pairwise

[15:08:47 CDT(-0500)] <Justin_o> Bosmon: so my new code that i passed there still has the same problem i mentioned as before, but does it at least address the other issues you've raised

[15:08:54 CDT(-0500)] <Bosmon> Your return value is assumed to be the target, or the "new target"

[15:09:10 CDT(-0500)] <Bosmon> You still have the issue that you assume that anything that isn't an array must be an object

[15:09:35 CDT(-0500)] <Bosmon> Which problem does it have?

[15:10:15 CDT(-0500)] <Bosmon> Presumably it doesn't still have the problem that the "extend" code never runs? : P

[15:11:26 CDT(-0500)] <Justin_o> Bosmon: yes.. that's gone, but the result is the same..

[15:12:00 CDT(-0500)] <Justin_o> i could remove that bit about the target always being an object.. i added that recently, not sure it's that helpful

[15:12:15 CDT(-0500)] <Bosmon> Well, it's already too late by there

[15:12:32 CDT(-0500)] <Bosmon> You've already embarked on a for (var key in source)

[15:12:36 CDT(-0500)] <Bosmon> Which may even crash immediately

[15:13:39 CDT(-0500)] <Justin_o> Bosmon: i guess that should be okay, since it does the array able check before hand

[15:13:50 CDT(-0500)] <Bosmon> How could it be ok? : P

[15:13:58 CDT(-0500)] <Bosmon> What if source is null

[15:14:17 CDT(-0500)] <Bosmon> Or indeed if it is anything which isn't an object

[15:14:39 CDT(-0500)] <Justin_o> Bosmon: oops.. yes.. my mistake.. was thinking that i had checked source, but it was target that was checked

[15:15:01 CDT(-0500)] <Bosmon> Even if you had checked source, almost all of the failure cases would still remain...

[15:15:34 CDT(-0500)] <Justin_o> Bosmon: interestingly though all my tests are passing and i do have cases where they are arrays

[15:15:55 CDT(-0500)] <Justin_o> i guess that's okay though

[15:16:35 CDT(-0500)] <Bosmon> I guess, for a start, you don't have any cases like this: model: "a"

[15:16:59 CDT(-0500)] <Justin_o> Bosmon: yep.. that's true..

[15:17:16 CDT(-0500)] <Bosmon> But now I'm puzzled - you say all your tests are passing, yet you "have the same problem as before"?

[15:18:16 CDT(-0500)] <Justin_o> Bosmon: well the tests for the extend function are all passing.. although you have pointed out some deficiencies in my testing..

[15:18:27 CDT(-0500)] <Justin_o> Bosmon: but the merging is having the same problem

[15:19:25 CDT(-0500)] <Justin_o> Bosmon: hmm.. actually it might be working now.. one second, may have forgotten to update that other test

[15:19:35 CDT(-0500)] <Bosmon> As a minor note, sometimes you are spelling "override" with 1 r

[15:19:53 CDT(-0500)] <Justin_o> Bosmon: it's cases like this that copy & paste is a good thing (sad)

[15:21:35 CDT(-0500)] <Justin_o> Bosmon: okay.. actually things are working now in my other unit test too.. had forgotten i changed one of the models and didn't update the expected value appropriately..

[15:22:00 CDT(-0500)] <Justin_o> i'll try to add some more tests to make sure i'm taking care of cases where the source target are other types

[15:23:11 CDT(-0500)] <Bosmon> cool - looks like it is coming on well

[15:23:54 CDT(-0500)] <Bosmon> As with my framework pull of last night, the main value of this exercise is likely to be the test cases rather than the specific implementation code : P

[15:24:03 CDT(-0500)] <Bosmon> Although having at least SOME kind of working implementation can't hurt....

[15:26:35 CDT(-0500)] <Justin_o> Bosmon: (smile)

[15:26:53 CDT(-0500)] <Justin_o> Bosmon: i'll be heading out soon, but hope to have something to show you tomorrow.. and lots of tests of course

[15:27:04 CDT(-0500)] <Bosmon> Justin_o - awesome

[15:27:40 CDT(-0500)] <Bosmon> I'll work on your renderer expander issue shortly

[15:28:36 CDT(-0500)] <Justin_o> Bosmon: thanks.. that would be great.. i have worked around it for the time being by using %0 and passing in the args as an array instead of the object directly..

[15:37:39 CDT(-0500)] <cindyli> anastasiac: michelled, one blocker http://issues.fluidproject.org/browse/FLOE-59 might be fixed. Can you try it to confirm?

[15:42:11 CDT(-0500)] <cindyli> confirmed. thanks, anastasiac

[15:51:00 CDT(-0500)] <NickMayne> Hey Hey Peeps

[15:53:05 CDT(-0500)] <NickMayne> hey @bosmon how tricks?

[15:54:17 CDT(-0500)] <michelled> anastasiac: does it matter where in the development.py file I set compress to true?

[15:54:44 CDT(-0500)] <anastasiac> michelled, I doubt it, but I've only tried it in one location, so I couldn't really say (smile) I put it at the bottom

[15:55:15 CDT(-0500)] <michelled> doesn't seem to be working for me (sad)

[16:02:59 CDT(-0500)] <Bosmon> Hey NickMayne (smile)

[16:03:05 CDT(-0500)] <Bosmon> So you say it fails for you in FF 14 as well?

[16:03:12 CDT(-0500)] <NickMayne> Yes it did

[16:03:15 CDT(-0500)] <NickMayne> I remember seeing it

[16:03:28 CDT(-0500)] <Bosmon> But was that just when it was a beta too?

[16:03:31 CDT(-0500)] <Bosmon> Or can you see it now?

[16:04:43 CDT(-0500)] <NickMayne> I dont have FF14 here, i only have 15

[16:04:57 CDT(-0500)] <NickMayne> But I can see it happening on 15 now.

[16:05:08 CDT(-0500)] <Bosmon> ok

[16:10:57 CDT(-0500)] <NickMayne> not that this helps but this is the error i get

[16:10:58 CDT(-0500)] <NickMayne> TypeError: nodes is undefined [Break On This Error] for (var i = 0; i < nodes.length; ++i) {

[16:11:41 CDT(-0500)] <NickMayne> Hmm aactually I get that when it loads

[16:11:44 CDT(-0500)] <NickMayne> let me check

[16:12:11 CDT(-0500)] <NickMayne> ignore that sorry

[16:53:47 CDT(-0500)] <NickMayne> yo @Bosmon what do you think the problem is?

[16:54:27 CDT(-0500)] <Bosmon> I'll have to install the beta at some point

[16:54:35 CDT(-0500)] <Bosmon> Right now my guess is it is their bug rather than ours (smile)