trigger: Decouple Trigger from clap #2864
Conversation
There was a problem hiding this comment.
In general I'm in favor of reducing the number of dependencies an implementor has to explicitly depend on especially if the implementor doesn't really have a choice.
I do feel though like we've long been on a path of exposing dependencies just when we run into a place where it would be convenient. This doesn't feel particularly disciplined, and I worry that over time we're going to regret some of these decisions.
I'm not necessarily against merging this as is, so I'll approve the PR, but I wanted to voice my concern that I'm not convinced this is the right path forward.
crates/trigger/src/lib.rs
Outdated
| use spin_factors_executor::{FactorsExecutorApp, FactorsInstanceBuilder}; | ||
|
|
||
| pub use anyhow; | ||
| pub use clap::Parser; |
There was a problem hiding this comment.
I don't love making clap even more a part of the Spin public API, but alas it may be too late...
There was a problem hiding this comment.
Another option would be to rename Trigger::CliArgs to e.g. ::GlobalConfig and move the Args constraint to FactorsTriggerCommand ... T::GlobalConfig: Args (and RuntimeFactorsBuilder? 🤔)
| @@ -4,10 +4,12 @@ pub mod loader; | |||
| use std::future::Future; | |||
|
|
|||
| use clap::Args; | |||
| use spin_core::Linker; | |||
| use spin_factors::RuntimeFactors; | |||
| pub use spin_core::Linker; | |||
There was a problem hiding this comment.
I'm surprised by this. I looked at a few of the external triggers, and they don't use Linker. Is this really necessary?
There was a problem hiding this comment.
Its used in the (admittedly optional) Trigger::add_to_linker signature.
Signed-off-by: Lann Martin <[email protected]>
- Rename `Trigger`'s associated `CliArgs` type to `GlobalConfig` - Move that `clap::Args` bound into `FactorsTriggerCommand` - Fix up related code Signed-off-by: Lann Martin <[email protected]>
lann
changed the title
Trigger's associatedCliArgstype toGlobalConfigclap::Argsbound intoFactorsTriggerCommandAlso (the original PR):