Minimize exposed API #29
Conversation
Signed-off-by: Javier G. Sogo <[email protected]>
jgsogo
force-pushed
the
api/stable-v0.1.0
branch
from
7af7744 to
9714156
Compare
| fn is_enabled(&self) -> crate::errors::Result<bool> { | ||
| self.client.get_feature(&self.feature_id)?.is_enabled() | ||
| } |
There was a problem hiding this comment.
for the future:
Not sure if is_enabled this is something we want to expose to the user. The concept of "enabled" is hard to understand and does not consider entity evaluation.
It might be confusing to see the feature as enabled but when getting the value (using entity) a user will see the disabled value, as then rollout percentage and segment evaluation is considered.
It might tempt users to use the is_enabled() function to evaluate the feature (assuming most features are boolean) which would give wrong results.
IMHO, the client should not provide this function, only admin interface server config should give control over this.
maybe @saikumar1607 can help to clarify?
There was a problem hiding this comment.
If a feature is disabled, the user can choose not to evaluate it for the entities, but it doesn't mean that the user knows the enabled and disabled values.
It might tempt users to use the is_enabled() function to evaluate the feature (assuming most features are boolean) which would give wrong results.
Only using our get_value, the user can retrieve the values.... and it requires the Entity:
fn get_value(&self, entity: &impl Entity) -> Result<Value>;And of course, even if the features are boolean and "disabled" means "false", the user won't benefit of the rollout related capabilities provided by IBM AppConfiguration.
In my mind, knowing if a feature is enabled or not, could be needed for applications that might choose whether to load a module or not. For example, given an e-commerce, if the "groceries feature" is not enabled, I won't load that module (maybe at startup time). Or, for example, if the "advanced-pricing" is disabled, I will just use default pricing for all the users instead of running some evaluation for each product for each user.
| pub(crate) struct Segments { | ||
| pub segments: Vec<String>, | ||
| } | ||
|
|
||
| #[cfg(test)] | ||
| pub mod tests { | ||
| pub(crate) mod tests { | ||
|
|
||
| use super::*; | ||
| use rstest::*; | ||
| use std::{fs, path::PathBuf}; | ||
|
|
||
| #[fixture] | ||
| pub fn example_configuration_enterprise() -> Configuration { | ||
| pub(crate) fn example_configuration_enterprise() -> Configuration { |
There was a problem hiding this comment.
Just for my understanding:
why sometimes
pub(crate)...{
pub ...
}
and sometimes
pub(crate)...{
pub(crate) ...
}
?
closes #7
Minimizes the exposed public API (removes some methods in the
FeatureandPropertytrait that were exposing privatemodels):In another PR, we could merge
Valueandentity::AttrValue, they offer basically the same functionality, wdyt?