Always return empty map instead of nil when conversion fails

[andig]

Fix #282, partially reverts #269

[CLAassistant]

Thank you for your submission! We really appreciate it. Like many open source projects, we ask that you sign our Contributor License Agreement before we can accept your contribution.
_{You have signed the CLA already but the status is still pending? Let us recheck it.}

[andig]

Friendly ping @sagikazarmark would appreciate feedback/fix.

[sagikazarmark]

Well...it's not an easy question. The library is not consistent about return values. I guess I tried to follow a pattern with that change.

I have to look through the library to figure out what pattern is used the most.

But I'd say relying on a return value when an error is returned is generally not a good idea.

[ccoVeille]

Well...it's not an easy question. The library is not consistent about return values. I guess I tried to follow a pattern with that change.

I have to look through the library to figure out what pattern is used the most.

But I'd say relying on a return value when an error is returned is generally not a good idea.

Yes ... but ... no.

The bug in evcc comes from viper.GetStringMapString

That calls cast.ToStringMapString

And when you look at the code, you can see the code of cast is the one ignoring the error.

https://github.com/spf13/cast/blob/v1.9.2/zz_generated.go

So based on that, reverting the previous behavior is the least we can do.

So I agree with @andig

[ccoVeille]

I feel like tests should be added to prevent such issues in the future

https://github.com/spf13/cast/blob/v1.9.2/zz_generated.go

These generated files could be tested, but it would require to update

https://github.com/spf13/cast/blob/v1.9.2/generator%2Fmain.go

Or maybe to manually create zz_test.go to test the few ones that could fail

Or simply add tests to every single methods involved in this file.

I'm unsure what would be the thing to do. Maybe both.

[andig]

Readme says

If input is provided that will not convert to that type, the 0 or nil value for that type will be returned.

I seem to recall it saying something about „useful zero value“, but can‘t find that anymore. So nil may be expected from that perspective.

I any case it is an unannounced, breaking behavior change causing bugs.

[sagikazarmark]

After sleeping on it, I don't actually see the harm in returning an empty map/slice upon error.

But I think it should be consistent across all functions:

• I like the "useful zero value" concept, we should add it to the readme (and at the same time properly define the behavior)
• Make sure that all functions returning a type with a nil zero value actually returns a "useful zero value"

@andig would you mind going through the library to make sure it's done?

[andig]

Afaikt, maps are the odd outlier. I wouldn't care about nil slices since I'd need to len- check them anyway before accessing elements and I can always append. maps are really different in that a nil map cannot be appended to, i.e. keys added.

would you mind going through the library to make sure it's done?

I've had a look in the past and it's beyond my grasp, sorry.

If you really want have nil maps that's probably a fair choice in terms of consistency, but imho warrants a major version and announcement to not get hit in the face ;).

[andig]

Much appreciated, thank you!

[sagikazarmark]

I'm not sure how this passed CI, but tests are breaking all over now.

[sagikazarmark]

Fixed

[andig]

I'm sorry, no idea :O