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

Give L.CircleMarker.setLatLng a return statement. #2206

Merged

Conversation

radicalbiscuit
Copy link
Contributor

Ever since e5bf57c, L.CircleMarker.setLatLng has not had a return statement. The API says it's supposed to return this. This branch resolves this issue.

This should be backpatched to v0.7 as a hotfix if possible. The issue does not exist in any release prior to v0.7.

@mourner
Copy link
Member

mourner commented Nov 19, 2013

It's not a critical issue, so won't backpatch. Also, I'm not certain that changing the order won't break anything. It would be better to just add a return this; statement in the code.

@radicalbiscuit
Copy link
Contributor Author

That would leave a release that doesn't match the API docs. Which, as you say, is not critical, but irksome that the documentation for the version doesn't match the behavior for the version.

I found this bug by using leaflet-locatecontrol which chains setLatLng with other L.CircleMarker functions. In that case, it halts execution along the chain since nothing is returned. I would imagine this to be a common usage scenario.

As for the order of execution, I'm not sure either. I can return the line to its previous position and add a separate return statement. But how would you feel about assigning the result of L.Circle.prototype.setLatLng.call(this, latlng) to a variable and returning that? The only reason I propose this is to preserve as much object oriented inheritance as possible.

@radicalbiscuit
Copy link
Contributor Author

The PR has been updated. Consider my previous comment. Thanks for providing and maintaining Leaflet!

@mourner
Copy link
Member

mourner commented Dec 2, 2013

No need to save inherited method result to variable and return that, setters in Leaflet should always return this.

mourner added a commit that referenced this pull request Dec 2, 2013
…ng-return

Give L.CircleMarker.setLatLng a return statement.
@mourner mourner merged commit 75c8133 into Leaflet:master Dec 2, 2013
@radicalbiscuit radicalbiscuit deleted the fix-circlemarker-setlatlng-return branch December 2, 2013 16:17
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants