change String.toInt and String.toFloat to return Maybe instead of Result

[gyzerok]

Whats done

Two little changes to the String module API.

-toInt : String -> Result String Int
+toInt : String -> Maybe Int

-toFloat : String -> Result String Float
+toFloat : String -> Maybe Float

Why?

For more consistency of API with other core modules.

String

• uncons : String -> Maybe (Char, String)

List

• head : List a -> Maybe a
• tail : List a -> Maybe (List a)

Dict

• get : comparable -> Dict comparable v -> Maybe v

Personally I was confused when saw this first time. One may say that we can't get an actual error message with maybe, but there is only one possible error here. And you don't return error for example for while trying to get list head, it's just redundant.

[eeue56]

You need to also change the Elm files, not just the native files.

[eeue56]

You need to also change the Elm files, not just the native files.

[gyzerok]

@eeue56 yeah, you are right. Stupid me..

Now fixed.

[gyzerok]

@eeue56 yeah, you are right. Stupid me..

Now fixed.

[eeue56]

I also think adding some tests to https://github.com/elm-lang/core/blob/dev/tests/Test/String.elm might be worthwhile

[eeue56]

I also think adding some tests to https://github.com/elm-lang/core/blob/dev/tests/Test/String.elm might be worthwhile

[gyzerok]

Done.

[gyzerok]

Done.

[pisys]

I'd keep the Result return type because transforming a String to some number is something which can fail for various errors, not just one. It's just that the current implementation does not serve more specific error messages than the one implemented.

Additionally String.toInt and String.toFloat are different to the other functions you mentioned. The reason why they return Maybe is because there is something returned maybe. Nothing is returned in cases where there is nothing. That's not an error.

But a String which can't be turned into a number isn't nothing, it's an error. This difference has to be reflected by the type.

Since the Result can be turned into a Maybe very easily (call Result.toMaybe), I see no need to change the API. IMHO.

[pisys]

I'd keep the Result return type because transforming a String to some number is something which can fail for various errors, not just one. It's just that the current implementation does not serve more specific error messages than the one implemented.

Additionally String.toInt and String.toFloat are different to the other functions you mentioned. The reason why they return Maybe is because there is something returned maybe. Nothing is returned in cases where there is nothing. That's not an error.

But a String which can't be turned into a number isn't nothing, it's an error. This difference has to be reflected by the type.

Since the Result can be turned into a Maybe very easily (call Result.toMaybe), I see no need to change the API. IMHO.

[gyzerok]

@pisys can you provide list of potential errors which you see so far?

[gyzerok]

@pisys can you provide list of potential errors which you see so far?

[pisys]

The point is that a failng String-to-number conversion is an error and not nothing irrespective of how many or what kind of errors there could be.

[pisys]

The point is that a failng String-to-number conversion is an error and not nothing irrespective of how many or what kind of errors there could be.

[jvoigtlaender]

@gyzerok, @pisys, you both might want to take this discussion into account. Note also that that linked issue being closed does not mean it has been resolved. In fact, it lives on as an item in https://github.com/elm-lang/core/issues/322. That item's name is:

#369 - clearly define the "use case" for Maybe and Result in APIs

I don't think it makes much sense to have the discussion that you are having here, until that general item has been dealt with and been decided on (involving Evan).

As a consequence, I'm inclined to close this PR here and to ask you to instead add mention of String.toInt and String.toFloat to the discussion at #369, so that they are taken into account when this issue is revisited via #322 at some point.

[jvoigtlaender]

@gyzerok, @pisys, you both might want to take this discussion into account. Note also that that linked issue being closed does not mean it has been resolved. In fact, it lives on as an item in https://github.com/elm-lang/core/issues/322. That item's name is:

#369 - clearly define the "use case" for Maybe and Result in APIs

I don't think it makes much sense to have the discussion that you are having here, until that general item has been dealt with and been decided on (involving Evan).

As a consequence, I'm inclined to close this PR here and to ask you to instead add mention of String.toInt and String.toFloat to the discussion at #369, so that they are taken into account when this issue is revisited via #322 at some point.

[jvoigtlaender]

So, can I close here?

[jvoigtlaender]

So, can I close here?

[evancz]

Yeah, this may be the right thing, but the decision will happen at a different time. It's an easy change, so it's not vital for it to be done via PR.

[evancz]

Yeah, this may be the right thing, but the decision will happen at a different time. It's an easy change, so it's not vital for it to be done via PR.