docs: deprecate contrib multi-provider in favor of SDK implementation #1664
Conversation
Summary of ChangesHello @Vishalup29, I'm Gemini Code Assist1! I'm currently reviewing this pull request and will post my feedback shortly. In the meantime, here's a summary to help you and other reviewers quickly get up to speed! This pull request focuses on deprecating the existing multi-provider within the Highlights
Using Gemini Code AssistThe full guide for Gemini Code Assist can be found on our documentation page, here are some quick tips. Invoking Gemini You can request assistance from Gemini at any point by creating a comment using either
Customization To customize Gemini Code Assist for GitHub experience, repository maintainers can create a configuration file and/or provide a custom code review style guide (such as PEP-8 for Python) by creating and adding files to a Limitations & Feedback Gemini Code Assist may make mistakes. Please leave feedback on any instances where its feedback is incorrect or counter productive. You can react with 👍 and 👎 on @gemini-code-assist comments. If you're interested in giving your feedback about your experience with Gemini Code Assist for Github and other Google products, sign up here. You can also get AI-powered code generation, chat, as well as code reviews directly in the IDE at no cost with the Gemini Code Assist IDE Extension. Footnotes
|
Vishalup29
changed the title
![gemini-code-assist[bot]](https://avatars.githubusercontent.com/in/956858?s=60&v=4){kind=small}
There was a problem hiding this comment.
Code Review
This pull request deprecates the contrib multi-provider in favor of the new core SDK implementation. The changes correctly mark the classes and update documentation. My review includes a critical fix for a compilation error caused by a conflicting import. I've also suggested adding the @Deprecated annotation to all deprecated classes to ensure compiler warnings are generated, and improving the Javadoc @deprecated tags to link directly to the new classes for better developer experience. A minor formatting suggestion for the README is also included.
Additionally, I noticed that the dev.openfeature.contrib.providers.multiprovider.Strategy interface is not deprecated, while its implementations are. Since it's a related public type, it should probably be deprecated as well to be consistent. As this file was not part of the changes, I'm mentioning it here for your consideration.
| import dev.openfeature.sdk.ProviderEvaluation; | ||
| import dev.openfeature.sdk.Value; | ||
| import dev.openfeature.sdk.exceptions.GeneralError; | ||
| import dev.openfeature.sdk.providers.multiprovider.MultiProvider; |
There was a problem hiding this comment.
This import will cause a compilation error because its simple name MultiProvider conflicts with the class defined in this file. Since the {@link} tag in the Javadoc uses the fully qualified class name, this import is not necessary and should be removed.
| * @deprecated Use the strategy from the core SDK multi-provider package instead. | ||
| */ | ||
| @Slf4j | ||
| @NoArgsConstructor |
There was a problem hiding this comment.
In addition to the @deprecated Javadoc tag, you should also add the @Deprecated annotation to the class. This will cause the compiler to generate warnings when the class is used, which is crucial for communicating deprecations effectively.
@NoArgsConstructor
@Deprecated| * @deprecated Use the strategy from the core SDK multi-provider package instead. | ||
| */ | ||
| @Slf4j | ||
| @NoArgsConstructor |
There was a problem hiding this comment.
In addition to the @deprecated Javadoc tag, you should also add the @Deprecated annotation to the class. This will cause the compiler to generate warnings when the class is used, which is crucial for communicating deprecations effectively.
@NoArgsConstructor
@Deprecated| * @deprecated Use {@link dev.openfeature.sdk.providers.multiprovider.MultiProvider} | ||
| * from the core SDK instead. | ||
| */ | ||
| @Slf4j |
There was a problem hiding this comment.
In addition to the @deprecated Javadoc tag, you should also add the @Deprecated annotation to the class. This will cause the compiler to generate warnings when the class is used, which is crucial for communicating deprecations effectively.
@Slf4j
@Deprecated| * evaluation should error and “bubble up” the individual provider’s error in the result. As soon as | ||
| * a value is returned by a provider, the rest of the operation should short-circuit and not call | ||
| * the rest of the providers. | ||
| * @deprecated Use the strategy from the core SDK multi-provider package instead. |
There was a problem hiding this comment.
To improve developer experience and provide a clear migration path, it's better to use a {@link} tag in the @deprecated Javadoc to point directly to the replacement class. This allows IDEs to provide quick navigation and refactoring suggestions.
| * @deprecated Use the strategy from the core SDK multi-provider package instead. | |
| * @deprecated Use {@link dev.openfeature.sdk.providers.multiprovider.FirstMatchStrategy} from the core SDK instead. |
| * First Successful Strategy. Similar to “First Match”, except that errors from evaluated providers | ||
| * do not halt execution. Instead, it will return the first successful result from a provider. If no | ||
| * provider successfully responds, it will throw an error result. | ||
| * @deprecated Use the strategy from the core SDK multi-provider package instead. |
There was a problem hiding this comment.
To improve developer experience and provide a clear migration path, it's better to use a {@link} tag in the @deprecated Javadoc to point directly to the replacement class. This allows IDEs to provide quick navigation and refactoring suggestions.
| * @deprecated Use the strategy from the core SDK multi-provider package instead. | |
| * @deprecated Use {@link dev.openfeature.sdk.providers.multiprovider.FirstSuccessfulStrategy} from the core SDK instead. |
Vishalup29
force-pushed
the
java_sdk_issue_1486
branch
2 times, most recently
from
fe3284c to
6e50cbb
Compare
|
@chrfwow @toddbaert @aepfli Can you pls take a look when you get a chance? |
...r/src/main/java/dev/openfeature/contrib/providers/multiprovider/FirstSuccessfulStrategy.java
Outdated
Show resolved
Hide resolved
Vishalup29
force-pushed
the
java_sdk_issue_1486
branch
from
6e50cbb to
9e7c4c8
Compare
Vishalup29
force-pushed
the
java_sdk_issue_1486
branch
3 times, most recently
from
7bff26a to
3496316
Compare
… redirect users to SDK implementation. Signed-off-by: vishalup29 <[email protected]>
Vishalup29
force-pushed
the
java_sdk_issue_1486
branch
from
3496316 to
bfdb951
Compare
3 participants
Description
Deprecate the contrib multi-provider in favor of the new core SDK implementation, while keeping existing users backwards compatible.
Changes:
dev.openfeature.contrib.providers.multiprovider.MultiProvider(and related types) as@Deprecated.dev.openfeature.sdk.multiprovider.MultiProviderin the core SDK.providers/multiprovider/README.mdwith a deprecation notice and usage guidancefor the SDK-based multi-provider.
but clearly communicates the migration path.
Related Issues
Related to open-feature/java-sdk#1486
Notes
multi-provider and redirects users to the SDK implementation.
multi-provider in the core SDK.
Follow-up Tasks
How to test
mvn testfrom the contrib repo root).@Deprecatedwarnings for contrib multi-provider classes.dev.openfeature.sdk.multiprovider.MultiProviderin the core SDK.