[OpenLayers-Dev] constructor signatures

[Tim Schaub]

christopher.schmidt at nokia.com wrote:
> On Jun 17, 2010, at 6:33 PM, ext Tim Schaub wrote:
>>     var popup = new OpenLayers.Popup.FramedCloud({
>>         location: xy,
>>         text: text,
>>         closeBox: true
>>     });
>>
>> I think the second form is more readable and less prone to errors.
>>
>> The one argument constructor is nothing new.  We're doing it already in 
>> some places in the library.  People coming from popular JavaScript 
>> frameworks are comfortable with this.
>>
>> In adding the WMTS layer, I created a constructor that takes a single 
>> argument.  If others agree that this is the way forward, I think there 
>> is sense in getting used to it before 3.0.
>>
>> I am bringing this up here because Chris objected to my commit of the 
>> WMTS layer and reopened the ticket.
>>
>> If anybody else has an opinion about the many versus one argument 
>> constructor, please weigh in.
> 
> In general, for 2.x, I am supportive of having required 
> arguments as seperate parameters, and optional arguments in a single
> object. Some of our existing classes -- popups being a big one :) --
> are obviously not following this convention.
> 
> I do not think it is unreasonable to make some things 'required' even
> if they are actually not technically required. In the WMS spec, the 
> Title attribute is required, even though there is no technical reason
> why this matters; it simply makes working with layers easier when
> they have names, rather than just positions in an array.
> 
> When creating a new subclass, One of the first things that I do is
> compare the Constructor signatures of the parent class to the
> subclass. If the parent has a constructor which is not a 'subset' of
> the child object, it usually means I've done something wrong. In the 
> case of Layer, the parent class has:
> 
>   Layer(name, options)
> 
> My convention for subclasses, then, would be that I could put things
> *between* 'name' and 'options', but that I could not get rid of those
> things; Layer.WMS has:
> 
>   Layer(name, params, options);
> 
> In all cases I'm aware of (Though I have not examined filters and rules
> extensively, so this may not be true in some portions of the code), 
> all of our subclassed items may add things after the 'required' 
> parameters, and before the 'options' parameter (which provides a
> space for non-required parameters). 
> 
> I am comfortable with 'name' being treated as a 'required' parameter
> of layers. I am in favor of maintaining the style of 'extending' 
> parameters following this pattern in 2.x; specifically, if a 
> parent class declares something 'required', the subclass should
> generally treat it as 'required' (and therefore a seperate parameter)
> unless doing so is significantly painful.
> 

To clarify here, specifically for the WMTS layer, these are the strictly 
required properties:

  * url
  * layer identifier
  * style identifier
  * matrixSet identifier

And who knows, we might get smarter in the future and find that we can 
derive sensible defaults for those*.  Anyway, at least four required 
arguments to start with.  And by required I mean that the layer will not 
render anything when added to a map if those arguments are not provided. 
  I'm putting together a patch for the layer that will make the 
constructor throw an error when the layer is not properly configured - 
to reduce the chance for user error.

Having a single argument for the WMTS layer constructor also makes 
things nicer for creating layers from parsed capabilities.  This is a 
detail specific to the WMTS layer, but I had it in mind when making the 
decision.  In general, a single configuration argument is more 
convenient to pass around, extend, serialize (assuming people want to 
persist app configuration), etc.

> Backing out on something that is one of our few points of consistency --
> Layers require a string as their first argument -- would make me 
> sad, but I'm not going to argue that we need to revert a change
> because of it. I just think it's a worthwhile convention to hold onto
> until we really go ahead and change many of our conventions for the
> Glorious Future.
> 
> Going forward into 3.x, I am okay with abandoning this convention
> in favor of making everything optional. I don't like it, personally, 
> but if this is common Javascript convention, I'm willing to go with 
> it. 
> 

I completely understand this.  I suspected this was about a change that 
would make you sad.

So, right now we have an opportunity.  We can create new constructors 
that are forward compatible, or we can create new constructors that 
follow old conventions (and need to be changed in the future).

I hear that you (Chris), Eric, and Bart favor going with old conventions 
rather than forward compatibility.

And I know I am making presumptions about what things will look like in 
the future.

I appreciate hearing the thoughts of others as well.

Tim

* If we wanted to, we could easily come up with a sensible default for a 
layer name (or title).  I think people are used to things like creating 
a new document and having it be named "untitled" or something similar. 
But this is a completely different topic.

> Apologies if I nitpicked on this one specific feature; I can understand
> that the WMTS layer was likely a lot of work, and am sure that it will
> help many people. I just wanted to comment on the convention change 
> because I thought it might actively trip people up.
> 
> Best Regards,


-- 
Tim Schaub
OpenGeo - http://opengeo.org
Expert service straight from the developers.