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

Support python-like token replacement and formatting #3358

Closed
wants to merge 1 commit into from

Conversation

danpat
Copy link

@danpat danpat commented Oct 12, 2016

Launch Checklist

This PR introduces advanced (Python str.format()-like) token formatting when using {fieldname} string replacement.

The change is backwards compatible with existing syntax, we very conveniently (deliberately?) chose the curly-bracket syntax that is basically compatible with Python's str.format().

The primary motivation for this change is number formatting: float and double fields encoded in vector tiles are currently unformatted. Values like 0.1 encoded as a float currently end up as text labels like 0.100000005932. This change allows you to specify number formatting precision in the Python syntax, like: {float_field::.2} for 2 decimal places.

There is an alternatively library at https://github.com/xfix/python-format, which seems to have better test coverage, but does not support using dictionary keys as field names.

  • briefly describe the changes in this PR
  • write tests for all new functionality
  • document any changes to public APIs
  • post benchmark scores
  • manually test the debug page

@mourner
Copy link
Member

mourner commented Oct 12, 2016

Does https://github.com/davidchambers/string-format support formatting numbers? Skimming through code/tests, I can't seem to find anything about it.

@danpat
Copy link
Author

danpat commented Oct 12, 2016

@mourner ah nuts, you're right - I was accidentally using a branch davidchambers/string-format#2 locally and I forgot it wasn't in the published module yet.

@mourner
Copy link
Member

mourner commented Oct 12, 2016

@danpat I'm worried that this PR is 2.5 years old without any signs of life or reaction from the maintainer. I think we should look for a different, better-maintained module.

@jfirebaugh
Copy link
Contributor

We're in general going to be reluctant to extend the semantics of the interpolation "mini-language", per mapbox/mapbox-gl-style-spec#104.

In particular, we know that people are already using token names that include a literal colon character, per mapbox/mapbox-gl-style-spec#362, so changes such as this would likely be backwards-incompatible in those cases.

@mourner
Copy link
Member

mourner commented Oct 12, 2016

@jfirebaugh what do you think about adding a precision spec property that affects all numbers in token replacement? E.g. 2 would round all numbers to 12.34. Not as configurable but probably OK for most cases.

@danpat BTW, is this reproducible for any double values? I'd generally avoid encoding anything in VT as float, but double should be mostly representable in text...

@danpat
Copy link
Author

danpat commented Oct 13, 2016

Ok, the existence of : in field names definitely looks like a showstopper for this syntax, this approach seems dead in the water.

@mourner yes, the same problem occurs with double types, often with even more egregious printing. I'm using float because I have often, several thousand properties encoded, and I'm only including data from 0.0 to about 100.0 in 0.1 increments, I have no need for double precision.

I could work around this by pre-formatting the numbers as string properties, but that's putting formatting logic in the tile generation - that's something that should be decided at render time IMO.

Definitely open to other suggestions for specifying numeric precision.

@mourner You're suggesting something like this ?

"layout" : { "text-field": "{cost}", "numeric-precision":2 }

Configurable on a per-layer basis would work pretty well I think. I can definitely see problems in the future if it's a style-global value.

@mourner
Copy link
Member

mourner commented Oct 13, 2016

—> #3361

@mourner mourner closed this Oct 13, 2016
@lucaswoj
Copy link
Contributor

I wonder what it'd look like to implement this functionality via mapbox/mapbox-gl-function#28

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants