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 reference to Integer.parse/2 in String.to_integer/1 #7924

Merged

Conversation

hauleth
Copy link
Contributor

@hauleth hauleth commented Jul 17, 2018

This should reduce confusion for newcomers about how function works and
what to use when you need to gracefully handle possible ill-formated
strings.

This should reduce confusion for newcomers about how function works and
what to use when you need to gracefully handle possible ill-formated
strings.
@hauleth hauleth changed the title Add reference ti Integer.parse/2 in String.to_integer/1 Add reference to Integer.parse/2 in String.to_integer/1 Jul 17, 2018
@@ -2186,13 +2186,20 @@ defmodule String do
@doc """
Returns an integer whose text representation is `string`.

`string` must be the string representation of a integer. In order to parse
Copy link
Member

Choose a reason for hiding this comment

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

“of an integer”

Inlined by the compiler.

## Examples

iex> String.to_integer("123")
123

String.to_integer("invalid data")
Copy link
Member

Choose a reason for hiding this comment

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

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I tried, and the problem is that compiler will catch that as this will always fail and it will produce compiler warning.

Copy link
Member

@josevalim josevalim Jul 18, 2018

Choose a reason for hiding this comment

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

@hauleth you can try "hiding" the input.

iex> String.to_integer(List.to_string(["123", "invalid"]))

Copy link
Member

Choose a reason for hiding this comment

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

I personally think that would make the documentation a bit hard to understand, and I think that should be the focus (not the actual doctest). So I'm 👍 for the #=> solution.

@josevalim
Copy link
Member

@whatyouhide apparently changing the String module via the GitHub UI has normalized UTF-8 strings and changed the module semantics. 😱

@whatyouhide
Copy link
Member

My screw up is harder to fix than I expected 😅 @hauleth, would you mind closing this PR and opening a new one with the same changes (plus the small change in my commit, which just adds an "an")? Sorry for the inconvenience :)

@hauleth
Copy link
Contributor Author

hauleth commented Jul 18, 2018

@whatyouhide I can force push my changes. I will fix this one ;)

@hauleth hauleth force-pushed the add-reference-to-parsing-function branch from ad4885b to 6f1b161 Compare July 18, 2018 15:53
@hauleth
Copy link
Contributor Author

hauleth commented Jul 18, 2018

@whatyouhide done.

Copy link
Member

@whatyouhide whatyouhide left a comment

Choose a reason for hiding this comment

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

Thanks for fixing it @hauleth! 😄

Inlined by the compiler.

## Examples

iex> String.to_integer("123")
123

String.to_integer("invalid data")
Copy link
Member

Choose a reason for hiding this comment

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

I personally think that would make the documentation a bit hard to understand, and I think that should be the focus (not the actual doctest). So I'm 👍 for the #=> solution.

@josevalim josevalim merged commit 14e4c49 into elixir-lang:master Jul 19, 2018
@josevalim
Copy link
Member

❤️ 💚 💙 💛 💜

@hauleth hauleth deleted the add-reference-to-parsing-function branch September 11, 2018 12:00
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Development

Successfully merging this pull request may close these issues.

4 participants