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

Add precision parameter to all toGeoJSON functions #5444

Merged

Conversation

mattgrande
Copy link

As recommended in PR #5237. Example usage:

var marker = new L.Marker([10.123456, 20.123456, 30.123456]);
marker.toGeoJSON();    // Works without changes
marker.toGeoJSON(3);   // Restricts all numbers to three significant digits

Copy link
Member

@perliedman perliedman left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Hi and thanks a lot for looking into this!

I have three points I'd like to see addressed before merging this:

  • As it is now, the coordinates output by toGeoJSON will be strings, not numbers; for example ["-87.359296", "35.00118"] rather than the expected [-87.359296, 35.00118] - this looks wrong and might possibly trip up some receivers that care about data types; it is also against the RFC 7947 which says: "A position is an array of numbers"
  • The GeoJSON spec gives the precision as number of decimals, while this PR gives the precision as number of significant digits; as it is now, it is not possible to give a number that adheres to the GeoJSON spec's recommendation of six decimals
  • Not necessarily important, but I think it might also be a good idea to default to the GeoJSON specs recommendation, so that we use six decimals unless something else is specified.

@mattgrande
Copy link
Author

Hmm, looks like toFixed would work better than toPrecision... but that also returns a string, and if I convert to a number, I'm getting weird results. Will continue to look into it.

@fminuti
Copy link
Contributor

fminuti commented Apr 21, 2017

I think you could use L.Util.formatNum

fminuti added a commit to fminuti/Leaflet that referenced this pull request May 3, 2017
Changed Util.formaNum default to 6 decimails.
Solved 0 decimals bug.
Added tests.
Can be useful for PR Leaflet#5444
@mattgrande
Copy link
Author

Sorry for the delay, @perliedman! Those changes you requested are in.

// Reverse of [`coordsToLatLng`](#geojson-coordstolatlng)
export function latLngToCoords(latlng) {
export function latLngToCoords(latlng, precision) {
precision = precision || 6;
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

this way you cannot round to 0 decimals. When (if) my pr #5492 lands, you can skip this line of code.

Copy link
Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Oh, good catch, I'll update that. I'm going to leave the default of 6 for now, though, because I don't know when/if your PR will come through.

Copy link
Member

@perliedman perliedman left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Looks good! Thanks for finishing this!

@perliedman perliedman merged commit 1120c46 into Leaflet:master May 31, 2017
cherniavskii pushed a commit that referenced this pull request Oct 2, 2017
* Util.formatNum default to 6

Changed Util.formaNum default to 6 decimails.
Solved 0 decimals bug.
Added tests.
Can be useful for PR #5444

* Changed documentation

Changed documentation to 6 instead of 5
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.

4 participants