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

Useless lifetimes #19

Closed
Bowarc opened this issue Sep 17, 2022 · 5 comments
Closed

Useless lifetimes #19

Bowarc opened this issue Sep 17, 2022 · 5 comments

Comments

@Bowarc
Copy link
Contributor

Bowarc commented Sep 17, 2022

Hellow, im tring to have some sort of dynamic Activity,
But the fact that activity::Activity uses Option<&'a str> instead of Option<String> + that it's returning self for each function makes that it's rly hard to use imo.
I made a config-like structure to be used in .ron files so i can add or remove parts easily
(I know it's supposed to be chained but i can't find a way to make it work with the optional config that i am using)
Example:

let mut activity = activity::Activity::new();
match activity_config.state {
    Some(state) => {
        debug!("Activity::State set to: {}", state);
        activity = activity.state(state);
    }
    None => debug!("Activity::State not found"),
}
match activity_config.details {
    Some(details) => {
        debug!("Activity::Details set to: {}", details);
        activity = activity.details(details);
    }
    None => debug!("Activity::Details not found"),
}

This is impossible with the current type of activity::Activity
The example above is working if you replace every Option<&'a str> by Option<String> and remove lifetimes on activity::Activity

It might just be me as im not very experienced with lifetimes, but i see no advantage of using Option<&'a str> instead of Option<String>.

@antiaim
Copy link

antiaim commented Sep 17, 2022

Agreed, was trying to run the ipc client in a thread but couldnt share the Activity struct with the thread because of the lifetimes. ended up having to just make another struct that contains the data and sharing that instead.

@vionya
Copy link
Owner

vionya commented Sep 17, 2022

First thing to disclose: it's been a hot minute since I implemented the Activity interface, so I unfortunately cannot tell you my original thought process behind the lifetimes and everything.

That aside, would it be preferable to make a breaking change wherein instead of chaining, it instead has set_* operations that modify it in-place?

let mut activity = Activity::new();
match activity_config.state {
    Some(state) => {
        debug!("Activity::State set to: {}", state);
        activity.set_state(state);
    }
    None => debug!("Activity::State not found"),
}
match activity_config.details {
    Some(details) => {
        debug!("Activity::Details set to: {}", details);
        activity.set_details(details);
    }
    None => debug!("Activity::Details not found"),
}

Thanks for the thread :)

@Bowarc
Copy link
Contributor Author

Bowarc commented Sep 18, 2022

Yes, why not.
Having functions like

Activity::set_state(&mut self, new_state: &str) -> () {
    self.state=new_state
}

Could be very helpfull for thoses kind of scenarios.

@vionya
Copy link
Owner

vionya commented Sep 18, 2022

Alright, I'll plan to replace the current system with something like that in the next major version.

@vionya vionya closed this as completed Sep 18, 2022
@Bowarc
Copy link
Contributor Author

Bowarc commented Sep 19, 2022

I tried a bit more and it turns out that, in my case, using the kw ref in the Some() section of the match compiles.

match activity_config.state {
    Some(ref state) => {
        debug!("Activity::State set to: {}", state);
        activity = activity.state(&state);
    }
    None => debug!("Activity::State not found"),
}
match activity_config.details {
    Some(ref details) => {
        debug!("Activity::Details set to: {}", details);
        activity = activity.details(&details);
    }
    None => debug!("Activity::Details not found"),
}

Even if that case worked; for other scenarios, having functions that take &mut self would be greatly appreciated.

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

3 participants