Make features.BindStruct a Variable or an Option, Instead of Build Constraints

motoki317 commented 3 months ago

Preflight Checklist

[X] I have searched the issue tracker for an issue that matches the one I want to file, without success.

Problem Description

The features.BindStruct feature flag (which is also the only one feature flag in this package) is currently implemented as a build constraint here: https://github.com/spf13/viper/blob/272344e4263af5a4463828db9e1862b6a35d812d/internal/features/bind_struct.go#L1-L5

Being implemented as a build constraint, users have to add a whole new viper_bind_struct build tag to their build scripts, in order to enable this feature. This was quite surprising (to me, at least), and I think it's making harder for people to use this feature.

Proposed Solution

Option 1: Make this feature flag a dynamically configurable variable instead of a const regulated by build constraints, and move the features package outside of the internal package.

package features

var BindStruct = false

This way, users of viper can dynamically enable this feature from normal golang source code, like as follows:

import "github.com/spf13/viper/features" // Just an example new package path

func main() {
    features.BindStruct = true // Enable the feature

    // Use the feature...
    var c MyConfig
    viper.AutomaticEnv()
    viper.Unmarshal(&c)
}

Option 2: Make this to an option that you can pass to viper.NewWithOptions, or something that you can configure later with viper instances. I believe this was the intention of this original TODO comment here: https://github.com/spf13/viper/blob/272344e4263af5a4463828db9e1862b6a35d812d/viper.go#L979

func main() {
    v := viper.NewWithOptions(viper.BindStruct()) // Enable the feature (just an example option name)

    // Use the feature...
    var c MyConfig
    v.AutomaticEnv()
    v.Unmarshal(&c)
}

or something similar to AutomaticEnv() usage:

func main() {
    var c MyConfig
    viper.BindStruct() // Enable the feature (just an example option name)
    viper.AutomaticEnv()
    viper.Unmarshal(&c)

    // or to a specific viper instance
    v := viper.New()
    v.BindStruct()
    v.AutomaticEnv()
    v.Unmarshal(&c)
}

Alternatives Considered

No response

Additional Information

Related issues and PRs:

https://github.com/spf13/viper/pull/1429
https://github.com/spf13/viper/issues/1797
https://github.com/spf13/viper/pull/1720 It seems this build constraint was introduced to address https://github.com/spf13/viper/issues/1706.
https://github.com/spf13/viper/issues/1721 If this issue is resolved, then issue/1721 may not be needed (as suggested in the first comment)

github-actions[bot] commented 3 months ago

👋 Thanks for reporting!

A maintainer will take a look at your issue shortly. 👀

In the meantime: We are working on Viper v2 and we would love to hear your thoughts about what you like or don't like about Viper, so we can improve or fix those issues.

⏰ If you have a couple minutes, please take some time and share your thoughts: https://forms.gle/R6faU74qPRPAzchZ9

📣 If you've already given us your feedback, you can still help by spreading the news, either by sharing the above link or telling people about this on Twitter:

https://twitter.com/sagikazarmark/status/1306904078967074816

Thank you! ❤️

sagikazarmark commented 3 months ago

How do you feel about #1854?

I'd imagine it would become the default behavior at some point, so the experimental flag would become noop.

motoki317 commented 3 months ago

How do you feel about #1854?

I'd imagine it would become the default behavior at some point, so the experimental flag would become noop.

Thank you for reviewing this quickly!

I looked at the PR changes, it looks mostly OK, but I guess the build tag will remain to control the behavior of the global viper instance?

I think it's best not to let users depend on custom build tags, even if they're experimental. Exposing these options as ExperimentalFoo() functions or options would be much easier to use than a build tag. In case the experimental options go away, it would break the builds for clients using the function so users can easily notice the breaking change.

So I think it would be more natural if the global and custom viper instances had similar ways to configure the behavior. This would also be similar with many existing options, such as: https://github.com/spf13/viper/blob/3266e43d900865ef1a2f72709119b0ae1a74eeed/viper.go#L1404-L1410 so how about:

func main() {
    // Enable for the global instance
    viper.ExperimentalBindStruct()
    // Enable for an instance
    v := viper.New()
    v.ExperimentalBindStruct()
}

Perhaps we can change all existing option names into something along WithFoo() in viper v2, so that the API looks more tidy and users won't be intimidated when they type viper. with autocompletes.

sagikazarmark commented 3 months ago

The build tag controls the default behavior. Incidentally, you are right: it also controls the global Viper behavior.

I try to discourage everyone from using the global Viper, though. I don't think it's good practice to use the global instance.

I'm also eliminating the state modification pattern: all new options should be passed through NewWithOptions instead of calling setters. I realize that's a challenge with the global instance. I haven't figured out a good solution there yet.

Let me think about that.

Is the global instance something you use? If so, why? Would it be difficult to migrate to a non-global instance?

motoki317 commented 3 months ago

Yes, I do use global instance of viper to build some applications. It's mainly because I only need a single instance in a whole application, and I think it's the same for most applications. It might not be too difficult to migrate to a non-global instance, but sometimes it's just easier to call the package methods directly. https://github.com/spf13/viper?tab=readme-ov-file#viper-or-vipers

I'm also eliminating the state modification pattern: all new options should be passed through NewWithOptions instead of calling setters. I realize that's a challenge with the global instance. I haven't figured out a good solution there yet.

I agree on that, but removing the current package-level methods and global instance would be too much of a change. Perhaps we can achieve that in v2.

sagikazarmark commented 3 months ago

Yes, that's a goal for v2.

What do you think about #1856 ?

motoki317 commented 3 months ago

Yes, that's a goal for v2.

What do you think about #1856 ?

I see, I think it's looking good as an option for configuring the global instance for v1.

andig commented 3 months ago

Copied from https://github.com/spf13/viper/pull/1854#issuecomment-2154166954:

Love it- much simpler to use than the compile switches. Doesn't this also replace https://github.com/spf13/viper/pull/1715? Wondering why that's not visible in the diff? However, it's not possible to use with the global viper instance, right?

spf13 / viper