-
Notifications
You must be signed in to change notification settings - Fork 185
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
feat(spells): add opt alias
arg to spell install [NET-529]
#1772
Conversation
@@ -372,7 +372,7 @@ impl ParticleAppServices { | |||
&self.aliases.read(), | |||
worker_id, | |||
self.config.local_peer_id, | |||
service_id_or_alias, | |||
service_id_or_alias.to_string(), |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I believe this is a bad pattern. Although it removes a lot of top-level clones, which makes the code look cleaner, it also introduces a deceiving function signature (ref => don't need to own) and deprives the ability to control your allocations on the top level.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
We can use Borrow
trait if you'd like to offer flexibility, but I don't see it as a bad pattern at all. I see only one case, but it can be resolved with cloning to closure
Description
Add an optional
alias
argument toSpell.install
.Motivation
Decider installs new installation spells and sets
worker-spell
aliases, and with this feature it will be more atomic and decider's Aqua code can be simpler.Proposed Changes
Add optional argument and remove spell if
add_alias
has failed.Checklist
Reviewer Checklist