Skip to content

Conversation

jgsogo
Copy link
Collaborator

@jgsogo jgsogo commented Dec 11, 2024

  • A new Feature trait that is implemented by our current Feature and FeatureProxy.

  • Rename our current Feature to FeatureSnapshot, to make it clear it will always return the same evaluation

  • Reimplement FeatureProxy making it much straightforward

    pub struct FeatureProxy<'a> {
        client: &'a AppConfigurationClient,
        feature_id: String,
    }

    with this approach, every method can be implemented in terms of our current Feature: we just return a regular feature (with the configuration at that time) and call the method on it.

        fn get_value(&self, entity: &impl Entity) -> crate::errors::Result<super::value::Value> {
            self.client.get_feature(&self.feature_id)?.get_value(entity)
        }

@jgsogo jgsogo changed the title Declare a Feature trait, implement FeatureProxy in terms of inner Feature Declare a Feature trait, re-implement FeatureProxy Dec 11, 2024
Comment on lines +25 to +26
pub struct FeatureProxy<'a> {
client: &'a AppConfigurationClient,
Copy link
Collaborator Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This makes it evident that the feature proxy can only live as long as the client is around. See the getter:

pub fn get_feature_proxy<'a>(&'a self, feature_id: &str) -> Result<FeatureProxy<'a>>

This implementation will also let us simplify the internals of the AppConfigurationClient... I'm not 100% sure if this will be the final API, but as long as we don't have a user requesting something different I would choose whatever is easier to implement/maintain.

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I like the reference here :-) How would a user use this though? is the proxy still useful?

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

as discussed: probably like this:

{
let client = AppConfigClient::new(...);
let feature1 = client.get_feature_proxy(...);
let feature2 = client.get_feature_proxy(...);
some_sub_component_entry_point(feature1);
some_other_sub_component(feature2);
}

Comment on lines +20 to +28
fn get_id(&self) -> &str;

fn get_name(&self) -> Result<String>;

fn get_data_type(&self) -> Result<crate::models::ValueKind>;

fn is_enabled(&self) -> Result<bool>;

fn get_enabled_value(&self) -> Result<crate::models::ConfigValue>;
Copy link
Collaborator Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I added these methods just because they are being used in some test or example, but I really think some of them don't belong to the API (or at least, I think we should not expose the models). This is something to tackle in #7

}

pub fn get_feature(&self, feature_id: &str) -> Result<Feature> {
pub fn get_feature(&self, feature_id: &str) -> Result<FeatureSnapshot> {
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

do you think we should change the function name to get_feature_snapshot too?

Copy link
Collaborator Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I don't think so, but I'm not sure. Maybe we should change FeatureSnapshot instead. Something like FeatureImpl or similar

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

No, I like the type name "FeatureSnapshot" :)

@rainerschoe rainerschoe merged commit 4645800 into IBM:main Dec 11, 2024
2 checks passed
@jgsogo jgsogo deleted the api/feature-trait branch December 11, 2024 16:03
jgsogo added a commit that referenced this pull request Dec 11, 2024
…roperty (#28)

See #26 for details

Signed-off-by: Javier G. Sogo <[email protected]>
@jgsogo jgsogo mentioned this pull request Dec 11, 2024
8 tasks
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

Successfully merging this pull request may close these issues.

2 participants