Skip to content

bug: arrays not being returned correctly #24

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
deadprogram opened this issue Mar 19, 2025 · 9 comments
Open

bug: arrays not being returned correctly #24

deadprogram opened this issue Mar 19, 2025 · 9 comments

Comments

@deadprogram
Copy link

deadprogram commented Mar 19, 2025

UPDATE: arrays of any type (int, string) are not being parsed correctly for any source format: JSON, TOML, and YAML.

When trying to populate a flag containing a string array coming from a TOML file, I discovered that instead of returning a slice with one string per array item, instead it is returning a slice with a single string that contains the entire array.

I added a unit test that demonstrates the problem in a fork/branch:

$ go test ./...
ok      github.com/urfave/cli-altsrc/v3 (cached)
ok      github.com/urfave/cli-altsrc/v3/json    (cached)
--- FAIL: TestTOMLArray (0.00s)
    toml_value_source_test.go:67: 
                Error Trace:    /home/ron/Development/cli-altsrc/toml/toml_value_source_test.go:67
                Error:          Not equal: 
                                expected: []string([]string{"hello", "hi", "hola", "hej"})
                                actual  : string("[hi hello hola hej]")
                Test:           TestTOMLArray
FAIL
FAIL    github.com/urfave/cli-altsrc/v3/toml    0.008s
ok      github.com/urfave/cli-altsrc/v3/yaml    (cached)
FAIL

Here is a commit with the failing test:
hybridgroup@8d09ba6

Thanks for any help!

@deadprogram
Copy link
Author

Additional note: I noticed that https://github.com/urfave/cli-altsrc/blob/main/toml/toml_map.go is no longer being used and should probably be removed.

@deadprogram deadprogram changed the title bug: string arrays not being returned correctly for TOML bug: string arrays not being returned correctly Mar 19, 2025
@deadprogram deadprogram changed the title bug: string arrays not being returned correctly bug: arrays not being returned correctly Mar 19, 2025
@dearchap
Copy link
Contributor

@deadprogram That seems to be correct. Would you like to open a PR and submit changes for this ?

@deadprogram
Copy link
Author

@dearchap what is the correct change to be made in your opinion? I don't mind helping out if I can get pointed in the correct direction.

@dearchap
Copy link
Contributor

@deadprogram Actually the behavior of toml is exactly as expected. It will return a string("[hi hello hola hej]") as a generic decode. If you specifically unmarshal it to a []string then it would give you the string slice as you want. The problem is that the string ("[hi hello hola hej]") will be set on flag and the parse will fail due to the square brackets. To overcome this we would need to update the main cli library to handle this case.

@deadprogram
Copy link
Author

@dearchap perhaps you can then point me towards the appropriate place in the main CLI for such a change?

@dearchap
Copy link
Contributor

dearchap commented Mar 31, 2025

https://github.com/urfave/cli/blob/main/flag_slice_base.go#L37

The other option is to check for slices in this library and strip away the square brackets for returning to the user.

@dearchap
Copy link
Contributor

dearchap commented Apr 5, 2025

@deadprogram Actually you dont need to change the cli code. You just need to change the code here https://github.com/urfave/cli-altsrc/blob/main/altsrc.go#L118 . We are returning a string(with brackets) from a slice. What you can do is remove the brackets from string if value is a slice

@deadprogram
Copy link
Author

deadprogram commented Apr 8, 2025

You just need to change the code here https://github.com/urfave/cli-altsrc/blob/main/altsrc.go#L118 .

@dearchap I tried what you suggested, however it did not appear to work.

@dearchap
Copy link
Contributor

dearchap commented Apr 9, 2025

@deadprogram Can you send a PR with the change ? We can look at it

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

No branches or pull requests

2 participants