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

Automatic string coercion for "concat" and text-field #6190

Closed
samanpwbb opened this issue Feb 17, 2018 · 13 comments
Closed

Automatic string coercion for "concat" and text-field #6190

samanpwbb opened this issue Feb 17, 2018 · 13 comments
Labels
api 📝 cross-platform 📺 Requires coordination with Mapbox GL Native (style specification, rendering tests, etc.)

Comments

@samanpwbb
Copy link
Contributor

mapbox-gl-js version: 0.44

It is common to want to use numbers as strings, especially in the text-field input. One use-case is to display number values on contour lines. This doesn't work:

["get", "ele"]

The proper expression is as follows:

["to-string", ["get", "ele"]]

For all users, this is pretty confusing, and doesn't seem necessary:

  • JS developers are used to numbers being treated like strings in cases like this, so I expect this will be a common stumbling block when implementing any expression on a string field.
  • In Studio, we can work around the current design by automatically wrapping all return values in the UI in to-string, but it adds complexity and just feels unnecessary.
@jfirebaugh
Copy link
Contributor

This is only really an issue for text-field, right? The other string-valued properties require values that are non numeral strings. (There's a corner case if you're using images with a numeric names in icon-image or *-pattern, but that seems uncommon.)

@mourner mourner added cross-platform 📺 Requires coordination with Mapbox GL Native (style specification, rendering tests, etc.) api 📝 labels Mar 15, 2018
@samanpwbb
Copy link
Contributor Author

This is only really an issue for text-field, right?

Mostly, but also effects image properties. For example, stylesheets often compose the name of a sprite image by looking at the # of characters in a data field. Would be nicer to be able to write get('type') & '-' & length(get('name')) instead of get('type') & '-' & to-string(length(get('name'))).

@jfirebaugh
Copy link
Contributor

I think that would be an issue with expression-jamsession? ["concat", ["get", "type"], "-", ["length, ["get", "name"]]] should work fine.

@jfirebaugh
Copy link
Contributor

Ah, that doesn't work. Okay, let's expand this to a request for "concat" to do auto-coercion.

@jfirebaugh jfirebaugh changed the title Coerce number values to strings in expressions on string property values Automatic string coercion for "concat" and text-field Mar 15, 2018
@samanpwbb
Copy link
Contributor Author

samanpwbb commented Apr 13, 2018

Another issue with the current implementation:

screen shot 2018-04-12 at 5 43 02 pm

to-string inside a coalesce never returns null. If I remove the to-string, I get what I expect:

screen shot 2018-04-12 at 5 43 10 pm

@samanpwbb
Copy link
Contributor Author

It would be great to add automatic string coercion *-image values as well as text-field.

@1ec5
Copy link
Contributor

1ec5 commented Apr 21, 2018

JS developers are used to numbers being treated like strings in cases like this, so I expect this will be a common stumbling block when implementing any expression on a string field.

This has also come up as a point of confusion for iOS developers, though the examples I’ve seen could’ve been mitigated through clearer documentation and error messages at the time.

@samanpwbb
Copy link
Contributor Author

samanpwbb commented Apr 23, 2018

@mapbox/gl-core automatic type coercion for text-field and image properties would solve some usability issues that have been recurring in Studio, so it would be amazing to see movement on this issue soon.

@jfirebaugh
Copy link
Contributor

jfirebaugh commented Apr 30, 2018

I implemented auto-coercion for concat in https://github.com/mapbox/mapbox-gl-js/compare/concat-to-string. But I'm not sure this is the right thing to do. If we really want auto-to-string there, and at the top level of string-typed properties, do we actually want it everywhere a string type is expected and a value is encountered? E.g.:

  • ["case", <some test>, "foo", ["get", "bar"]] (auto conversion for "bar")
  • ["match", ["get", "foo"], "a", "a-const", "b", ["get", "b-value"]] (auto conversion for "b-value")

In other words, treat the string type more like the color type, with implicit conversions rather than implicit assertions. But that feels like a more fundamental shift in our thinking than what's been previously proposed. I'm pretty sure we don't want to go down the path of making every type implicitly convertible, so I'd like to have a cogent argument why string is special.

@anandthakker
Copy link
Contributor

Some examples of undesirable results if we always did implicit conversion for strings:

  • ["match", ["get", "x"], "1", "yes", "no"] would then evaluate to "yes" for both {"x": 1} and {"x": "1"}
  • ["match", ["get", "x"], "", "yes", "no"] would evaluate to "yes" for all three of {}, {"x": ""}, and {"x": null}

Not sure if this is worse than the confusion that might be caused by having sometimes auto-coercion / sometimes assertion 🤔

@1ec5
Copy link
Contributor

1ec5 commented May 2, 2018

The examples in #6190 (comment) point to a possible need for a distinction between == and === if we were to perform automatic string coercion. Then again, maybe match isn’t relevant here: in JavaScript and other languages, switch has === semantics.

@kkaefer
Copy link
Member

kkaefer commented May 22, 2018

Looking at the various examples and counter-examples posted here, I think a reasonable compromise could be to coerce values to the expected type if we can deduce that type from the function. E.g. for case and match, the expected type of arguments is the deduced type of the test expression, and for concat, the expected type is always string.

let's expand this to a request for "concat" to do auto-coercion.

Is there any reason to not add an automatic string coercion for the text-field, icon-image, and *-pattern properties while we figure out what kind of string coercion we want to do in other cases?

@nickidlugash
Copy link

A related issue I've come across in styles that use Mapbox Streets data:

We are getting these warnings thrown:
Expected value to be of type string, but found null instead.

...because it is common for text-field values similar to ["get", "name"] to evaluate to null. With the previous token syntax, {name} did not seem to throw this error. It seems ineloquent to require an explicit coercion here (or to require coalesces with an empty string).

Is there any reason to not add an automatic string coercion for the text-field, icon-image, and *-pattern properties while we figure out what kind of string coercion we want to do in other cases?

I think this would solve our issues 👍

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
api 📝 cross-platform 📺 Requires coordination with Mapbox GL Native (style specification, rendering tests, etc.)
Projects
None yet
Development

No branches or pull requests

7 participants