Skip to content

fixes expected cast.ToInt behavior with prefixed "0" in string #75

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

Open
wants to merge 3 commits into
base: master
Choose a base branch
from

Conversation

cbajohr
Copy link

@cbajohr cbajohr commented Mar 12, 2019

Fixes issue #74

It fixes the expected behavior of cast.ToInt() with a string prefixed with "0".

Before this fix cast.ToInt("01234") results into 668 instead of 1234.
Now it converts strings like "1234" and "01234" to 1234.

There should be a separate cast method for hex integers like ("0x1234") like cast.ToHexInt("0x1234") e.g. if that is a must have.

@CLAassistant
Copy link

CLAassistant commented Mar 12, 2019

CLA assistant check
All committers have signed the CLA.

@bep
Copy link
Collaborator

bep commented Mar 18, 2019

If base == 0, the base is implied by the string's prefix: base 16 for "0x", base 8 for "0", and base 10 otherwise.

I did not originally implement this, but I don't think we can change the existing behavior, even if it may sound odd/wrong. A 0-prefixed integer means base 8. So if you want base 10 I think you need to trim those strings yourself.

@spf13 may chime in.

@cbajohr
Copy link
Author

cbajohr commented Mar 18, 2019

Maybe you are right, this could be a breaking change on existing implementations.
The thing is, that it's not obvious that this method converts the string to a base 8 instead of a base 10. At least it led to a misbehavior on one of my apps.

An optional base parameter for cast.ToInt could be a compromise. Or another method like cast.ToInt10() or something like this.

But yes, I also could cast the string myself.

@bep
Copy link
Collaborator

bep commented Mar 18, 2019

@cbajohr I'm pretty sure we have user-facing issues in Hugo regarding this, so we should definitively fix it ... somehow.

/cc @moorereason

@moorereason
Copy link
Contributor

This feature/issue has come up before (#56). Hugo docs are pretty clear: https://gohugo.io/functions/int/.

This PR would break the existing API by forcing all inputs to base 10, so I'm against merging it without a major semver release.

The "bug" is in the godoc comments. I'd recommend updating the godoc comments for one function (ToIntE) and then add a note to every relevant function to see ToIntE about base parsing.

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