OpenLayers OpenLayers

Ticket #992 (closed feature: fixed)

Opened 1 year ago

Last modified 1 year ago

Allow applyDefaults() to return the modified object

Reported by: fredj Assigned to:
Priority: minor Milestone: 2.6 Release
Component: Util Version: SVN
Keywords: Cc:
State:

Description

OpenLayers.Util.applyDefaults(to, from) doesn't return the modified object (since r1255).

I know it's redundant to return it but can be useful in the case to 'to' object in anonymous, for example: 

var my_style = OpenLayers.Util.applyDefaults({strokeColor: '#00ff00', 
                                              strokeOpacity: 0.4 }, 
                                              OpenLayers.Feature.Vector.style['default']);

Attachments

applyDefaults_return.patch (1.4 kB) - added by fredj on 09/18/07 09:33:37.
return the 'to' object, unit tests

Change History

09/18/07 09:33:37 changed by fredj

  • attachment applyDefaults_return.patch added.

return the 'to' object, unit tests

09/18/07 09:35:39 changed by fredj

  • keywords set to review.

(follow-up: ↓ 3 ) 09/18/07 11:51:03 changed by crschmidt

  • milestone set to 2.6 Release.

Good for discussion after 2.5.

(in reply to: ↑ 2 ) 11/05/07 14:19:08 changed by elemoine

Replying to crschmidt:

Good for discussion after 2.5.

fredj's patch makes sense to me. OpenLayers.Util.applyDefaults() should return the destination value, just like OpenLayers.Util.extend() does. Does anyone have a strong opinion against that? If not, I think we can commit this back to trunk.

11/05/07 14:23:28 changed by euzuro

patch makes sense to me too. eric if you can confirm that all tests are passing in ie/ff, then totally go ahead and approve/apply it :-)

11/05/07 16:19:46 changed by crschmidt

My only concern was that I seem to recall we used to *do* this, and then we stopped, because we had a mix of code depending half on the return value, half ont the values being updated in place. I seem to recall Erik being the one who complained about that :) If he's happy, Im' happy.

(follow-up: ↓ 7 ) 11/05/07 16:22:22 changed by euzuro

jaja. this does seem like the kind of thing that i'd complain about. as eric puts it, though, it seems to make sense to me. if we return a value in extend(), we might as well return one in applyDefaults() as well.

In theory, I think it's confusing, because the user might think s/he's getting a *new* object back.... but still. i think consistency is more important.

(in reply to: ↑ 6 ; follow-up: ↓ 8 ) 11/05/07 16:25:34 changed by elemoine

Replying to euzuro:

jaja. this does seem like the kind of thing that i'd complain about. as eric puts it, though, it seems to make sense to me. if we return a value in extend(), we might as well return one in applyDefaults() as well.

yeah, and there's is this anonymous dest object thing mentioned by fredj.

In theory, I think it's confusing, because the user might think s/he's getting a *new* object back.... but still. i think consistency is more important.

Tests pass on FF. As usual I don't have IE handy. I'll let fredj run the tests on IE.

(in reply to: ↑ 7 ) 11/09/07 07:56:45 changed by fredj

Replying to elemoine:

Replying to euzuro:

jaja. this does seem like the kind of thing that i'd complain about. as eric puts it, though, it seems to make sense to me. if we return a value in extend(), we might as well return one in applyDefaults() as well.

yeah, and there's is this anonymous dest object thing mentioned by fredj.

In theory, I think it's confusing, because the user might think s/he's getting a *new* object back.... but still. i think consistency is more important.

Tests pass on FF. As usual I don't have IE handy. I'll let fredj run the tests on IE.

All tests pass on IE 6

11/09/07 15:13:33 changed by elemoine

  • keywords deleted.
  • status changed from new to closed.
  • resolution set to fixed.

(In [5162]) Make OpenLayers.Util.applyDefaults() return the modified object. To be consistent with OpenLayers.Util.Extend() and be able to use anonymous object as the 'to' object. Thanks fredj for the patch and tests. Thanks euzuro and crschmidt for the reviews. (closes #992)