Ticket #2354 (closed bug: fixed)

Opened 4 years ago

Last modified 4 years ago

Spelling of API methods getURL and setUrl of imagelayers is inconsistent and confusing

Reported by: marcjansen Owned by: bartvde
Priority: minor Milestone: 2.9 Release
Component: Layer.Image Version: 2.8
Keywords: Cc:
State: Needs Discussion

Description

Every instance of OpenLayers.Layer.Image has the following two methods to set/get the URL of the layer:

  • getURL
  • setUrl

The spelling is inconsequent (Url vs. uppercase-only URL) and therefore rather confusing.

Attached is a patch that renames the method setUrl to setURL and remains a deprecated copy of the method setUrl so that existing applications using the old method don't break.

Attachments

imagelayer-method-spelling.patch Download (0.7 KB) - added by marcjansen 4 years ago.
the patch that changes the spelling of the method setUrl to setURL
imagelayer-method-spelling-2.patch Download (1.1 KB) - added by marcjansen 4 years ago.
The patch that changes the spelling of setUrl to setURL and issues a deprecated-warning when the old method is called.

Change History

Changed 4 years ago by marcjansen

the patch that changes the spelling of the method setUrl to setURL

Changed 4 years ago by bartvde

Marc, you are absolutely right, but personally I would rather leave this until OpenLayers 3.0. What do others think?

Changed 4 years ago by bartvde

  • milestone changed from 2.9 Release to 3.0 Release

Not a regression so bumping. I am taking this to the 3.0 milestone since I think it is more logical to change it there and then. Move back to 2.10 if you or some else disagrees.

Changed 4 years ago by pspencer

for instances like this, would it be useful to add a deprecation utility method, perhaps OpenLayers.Function.deprecated() so that you can do something like: getUrl: function() { /* the original implementation, renamed for consistency */ }, getURL: OpenLayers.Function.deprecated(OpenLayers.Layer.Image.getUrl),

where OpenLayers.Function.deprecated would issue a warning via console or alert depending on some global setting and then call the original function?

Changed 4 years ago by marcjansen

  • milestone changed from 3.0 Release to 2.9 Release

pspencer, this is more or less what the attached patch does: it keeps the old method and internally calls the new one. It does not, however, issue a dprecated-warning of any kind. The API-documentation for the old method contains a textual warning though.

The patch does not break the API, so I would love to have it in 2.9. As for the next version (2.10 or 3.0) the old (in 2.9 deprecated) and confusing method could be removed IMO.

  • 2.8: method there and inconsistent/confusing
  • 2.9: method there but calls a more guessable method and marks the confusing one as deprecated
  • 2.10/3.0: only the sane method is kept

Just my two cents. If people disagree on this, the version for this can be changed again. I promise to leave the ticket untouched if it is moved to future again :-)

Changed 4 years ago by bartvde

We do have a mechanism in place to warn for deprecation e.g.:

        var msg = "The OpenLayers.Layer.WMS.Untiled class is deprecated and " +
                  "will be removed in 3.0. Instead, you should use the " +
                  "normal OpenLayers.Layer.WMS class, passing it the option " +
                  "'singleTile' as true.";
        OpenLayers.Console.warn(msg);

Marc, can you update your patch with something like this?

Changed 4 years ago by marcjansen

The patch that changes the spelling of setUrl to setURL and issues a deprecated-warning when the old method is called.

Changed 4 years ago by bartvde

  • keywords spelling, renaming removed
  • status changed from new to closed
  • state changed from Review to Complete
  • resolution set to fixed

(In [10112]) Spelling of API methods getURL and setUrl of imagelayers is inconsistent and confusing, p=marcjansen, r=me (closes #2354)

Changed 4 years ago by bartvde

Thanks Marc, I am gonna open up a new ticket for 3.0 (ticket:2531) since there is still inconsistency between classes about URL versus Url, e.g. HTTPRequest uses setUrl.

Changed 4 years ago by tschaub

  • status changed from closed to reopened
  • state Complete deleted
  • resolution fixed deleted

Sorry I didn't comment on this earlier. I think this change was a bit short-sighted. It would be a change of less consequence to deprecate getURL on the image layer. All other layers that inherit from HTTPRequest get  setUrl.

All other getURL methods on layers are not marked as part of the API. The Image layer is the only layer with a getURL API method. If you want to deprecate something, deprecate that.

Changed 4 years ago by bartvde

  • owner changed from tschaub to bartvde
  • status changed from reopened to new

Thanks Tim, right I had noticed the inconsistency with HTTPRequest, but I thought URL would be a better way to write than Url. Are you suggesting to make getURL a non-api method as the only change here and reverting r10112 ?

Changed 4 years ago by tschaub

  • state set to Needs Discussion

Given that the image layer API has setUrl and getURL, and all other HTTPRequest layers have setUrl (and no public getURL), the simplest way to get consistency in the image layer API is to add getUrl and deprecate getURL. Adding setURL to the image layer API makes it inconsistent with every other HTTPRequest layer (which feels like a bigger inconsistency to me).

One alternative is to do nothing. When we do a more thorough review of the API, we can discuss whether we like setURL or setUrl and make a consistent change throughout (adding a new method and deprecating the old if there is sufficient motivation).

Anyway, in order of sensibility (to me), the alternatives are A) to do nothing with the image layer until we do a thorough renaming toward 3.0, B) to add getUrl to the image layer (making its API consistent), or C) to add setURL to the image layer (making it inconsistent with every other HTTPRequest layer.

Alternatives A and B involve reverting r10112.

Changed 4 years ago by bartvde

Ok thanks for the thorough explanation Tim, I am in favour of A or B (I do not have a real preference here as I said before my initial intention was to leave all this to 3.0, but I can also see the confusion).

Marc do you have enough interest in B) ? If so let me know and I'll change the code.

Changed 4 years ago by marcjansen

I wouldn't mind if r10112 was reverted. And bumping this up to the future wouldn't hurt my feelings either. I agree that the change in r10112 introduces as much inconsistency as it removes, at least when compaired to layers different from Layer.Image.

To get this issue closed I'd suggest

  • reverting r10112
  • creating a new ticket concerning the URL- vs. Url-methods throughout the library (setting the milestone to 2.10 or 3.0)
  • closing this ticket with a reference to the new one

If someone is really annoyed from the minor inconsistency in Layer.Image that this ticket originally adressed, he/she can probably do the needed changes by hand.

Changed 4 years ago by bartvde

  • status changed from new to closed
  • resolution set to fixed

reverted in r10139

Marc, I had already opened up a new ticket, see ticket:2531 for the future issue. Feel free to add comments there.

Thanks.

Note: See TracTickets for help on using tickets.