bugfix: fix an int parsing bug in godotenv.Marshal

[t3mp14r3]

If you encounter a value like +1234567890, in my case it was a phone number, the strconv.Atoi will actually remove the + sign and won't throw an error.

That's how it looks like:

x, err := strconv.Atoi(`+123456789`)
fmt.Println(err)
fmt.Println(x)

<nil>
123456789

Additionally, if you have a very long value, the strconv.Atoi will throw an error: value out of range, which is correct, but we shouldn't care, it's all numbers - just store it. And when reading back we get all values in string anyway.

Of course, in the case of godotenv this behavior may cause some issues (it did in my case), you can see my proposition for a fix in the commit.

After the change, only values containing numbers and nothing else will be treated as integers (without quotes), everything else will be in quotations.

Though using regex is not the most optimal solution, it was the first one, that came to me (and it's quite concise), but of course, it could be replaced with a better solution.

[ccoVeille]

I'm sorry, but while I agree there is a problem with current code. The way Atoi behaves with the + is indeed a surprise I would not expect something like this.

So yes, there is a problem with the +

But your solution also nuke the possibility for an integer to be negative.

Think about a variable set to -1 to inhibit a behavior, like a timeout or a max size

You may want to change the regexp to allow a possible leading -

But it would be a adding a fix on a solution that is not good. Regexp are slow also.

So, OK there is a problem with a leading +

First, let's add test for this, then add test for -1.

Then create a isInt method that will check if the first character is a + by using

strings.HasPrefix(n, "+")

If true, isInt will return false, if not strconv.Atoi can be used

[ccoVeille]

The regexp variable should be put outside the Marshall method, so at the package level.

Parsing a regexp has a cost, and here is would be done on each call to Marshall

[t3mp14r3]

Yep, you're completely right, I will deal with the possibility of negative numbers get rid of regex.

[ccoVeille]

Also, if you want to check a variable only contain number, you can use strings.Trim and provide the 0123456789 as argument.

If there is something remaining it's not a number whatever it's length.

But you might have to use strings.TrimPrefix first to remove the -

[t3mp14r3]

Sorry, I've noticed an error, just after the push, let me fix it in the next commit

[t3mp14r3]

I've made the changes (though, not exactly as you've suggested).

1. We first attempt to trim the leading "-" character (will remove only one character)
2. Check if string is empty
3. Then, we begin checking the remaining string for non digit characters

I've added a test to check various inputs, all goes through smoothly.

I think this should work, but please, if I've made a mistake, point it out.

Thanks!

[ccoVeille]

good idea, unfortunately you will have an issue with unicode.IsDigit

unicode is wider than what we think

package main

import (
	"fmt"
	"unicode"
)

func main() {
	fmt.Printf("%t\n", unicode.IsDigit('1'))
	fmt.Printf("%t\n", unicode.IsDigit('৩'))
	fmt.Printf("%t\n", unicode.IsDigit('A'))
}

I think you should replace your code with only a portion of what is in unicode.IsDigit

https://cs.opensource.google/go/go/+/refs/tags/go1.23.0:src/unicode/digit.go;l=8

so simply this

if '0' <= r && r <= '9' {
    continue
}
return false

[t3mp14r3]

That was a great catch, thanks! Well, it looks like everything is in order.

Should I squash my commits together to keep the history clean?

[ccoVeille]

Please format the file with gofmt, there is something strange with the code now

[t3mp14r3]

Yeah, something weird was going on, fixed that

[ccoVeille]

Please add a comment about the function purpose, people may be surprised about the fact you didn't use strings.Atoi

• the fact it supports big numbers with more than X digits
• the fact a number starting with + is not a valid number (and more likely to be a phone)

[t3mp14r3]

Added the comment, though it doesn't seem necessary (other internal functions don't have them), but let's let it be.

[ccoVeille]

Please add a test about a very long number that would break max int64

It's part of your code change

[t3mp14r3]

The update is here