Skip to content
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

README Example Outdated: Incorrect Package Reference for NewAPIConfig #56

Open
moesy opened this issue May 28, 2024 · 4 comments
Open

Comments

@moesy
Copy link

moesy commented May 28, 2024

The README example for using the Confidence SDK with OpenFeature appears to be out of date. Specifically, the lines:

import (
    "github.com/open-feature/go-sdk/openfeature"
    confidence "github.com/spotify/confidence-sdk-go/pkg/provider"
)
....

provider, err := confidence.NewFlagProvider(confidence.NewAPIConfig("clientSecret"))

is incorrect because the NewAPIConfig function is not part of the github.com/spotify/confidence-sdk-go/pkg/provider package. Instead, it is located in the github.com/spotify/confidence-sdk-go/pkg/confidence package.

nicklasl added a commit that referenced this issue May 28, 2024
nicklasl added a commit that referenced this issue May 28, 2024
@nicklasl
Copy link
Member

Thanks for taking the time to report this, we'll take care of it right away 👍

@moesy
Copy link
Author

moesy commented May 28, 2024

Thank you @nicklasl for the quick response!

The proposed solution works however your variable names collide with the package names so the go linters are going to complain a bit.

I would recommend either changing

confidence := confidence.NewConfidenceBuilder().SetAPIConfig(confidence.APIConfig{APIKey: "clientSecret"}).Build()
provider := provider.NewFlagProvider(confidence)

to something like:

confidenceApp := confidence.NewConfidenceBuilder().SetAPIConfig(confidence.APIConfig{APIKey: "clientSecret"}).Build()
confidenceProvider := provider.NewFlagProvider(confidenceApp)

The import aliases e.g: confidence "github.com/spotify/confidence-sdk-go/pkg/confidence" also seem redundant since the package names are the same. if you wanted to keep your variable names as confidence and provider you should change the aliases to another name.

@nicklasl
Copy link
Member

Ahh! thanks!

I'm thinking, since this is a readme, I may keep the import aliases to signal where the functions come from.

@moesy
Copy link
Author

moesy commented May 28, 2024

Keeping the alias sounds fine for readability - thanks again! The variable names assigned later in the example should change then to stop the name collisions. Other than that I can confirm everything works as intended.

nicklasl added a commit that referenced this issue May 28, 2024
* docs: fixup docs with correct imports

fixes issue #56

* fixup! docs: fixup docs with correct imports
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