-
Notifications
You must be signed in to change notification settings - Fork 56
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
Proc macro for tedge config #1936
Changes from 1 commit
375bc6c
fb59fda
5e9b80d
f4c67a1
1aab677
771db4c
80fe910
ffff8d1
8d59006
c7490e9
1868294
86af093
28dcee3
8cd46ad
6471e1f
98f4a85
6e2e3d0
6556d1c
98e5201
5b513a0
4696954
5d3e4e4
81b7193
ce77b22
b2ef8fa
3b95375
501b756
d2063d5
File filter
Filter by extension
Conversations
Jump to
Diff view
Diff view
There are no files selected for viewing
Original file line number | Diff line number | Diff line change |
---|---|---|
@@ -0,0 +1,87 @@ | ||
use crate::OptionalConfig; | ||
|
||
pub fn all_or_nothing<T, U>( | ||
t: OptionalConfig<T>, | ||
u: OptionalConfig<U>, | ||
) -> Result<Option<(T, U)>, String> { | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Definitely simpler and better to have this defined as a regular function instead of a macro. Beyond this specific case, I see this simplification as a better separation of the responsibilities. Adding constraints on what makes sense should be done at the application level while the config should be focus on a more syntactic level (what is set, what is missing, what is ill-formed). |
||
use OptionalConfig::*; | ||
|
||
match (t, u) { | ||
(Present { value: t, .. }, Present { value: u, .. }) => Ok(Some((t, u))), | ||
(Empty(..), Empty(..)) => Ok(None), | ||
(t, u) => { | ||
let all_settings = [t.key(), u.key()]; | ||
let present = [t.key_if_present(), u.key_if_present()] | ||
.into_iter() | ||
.filter_map(|id| id) | ||
.collect::<Vec<_>>(); | ||
let missing = [t.key_if_empty(), u.key_if_empty()] | ||
.into_iter() | ||
.filter_map(|id| id) | ||
.collect::<Vec<_>>(); | ||
Err(format!( | ||
"The thin-edge configuration is invalid. The settings {all_settings:?} must either all be configured, or all unset. Currently {present:?} are set, and {missing:?} are unset.")) | ||
} | ||
} | ||
} | ||
|
||
#[cfg(test)] | ||
mod tests { | ||
use super::*; | ||
|
||
#[test] | ||
fn all_or_nothing_returns_some_when_both_values_are_configured() { | ||
assert_eq!( | ||
all_or_nothing( | ||
OptionalConfig::Present { | ||
value: "first", | ||
key: "test.key" | ||
}, | ||
OptionalConfig::Present { | ||
value: "second", | ||
key: "test.key2" | ||
} | ||
), | ||
Ok(Some(("first", "second"))) | ||
) | ||
} | ||
|
||
#[test] | ||
fn all_or_nothing_returns_none_when_both_values_when_neither_value_is_configured() { | ||
assert_eq!( | ||
all_or_nothing::<String, String>( | ||
OptionalConfig::Empty("first.key"), | ||
OptionalConfig::Empty("second.key"), | ||
), | ||
Ok(None) | ||
) | ||
} | ||
|
||
#[test] | ||
fn all_or_nothing_returns_an_error_if_only_the_first_value_is_configured() { | ||
assert!(matches!( | ||
all_or_nothing::<_, String>( | ||
OptionalConfig::Present { | ||
value: "test", | ||
key: "first.key" | ||
}, | ||
OptionalConfig::Empty("second.key"), | ||
), | ||
Err(_) | ||
)) | ||
} | ||
|
||
#[test] | ||
fn all_or_nothing_returns_an_error_if_only_the_second_value_is_configured() { | ||
assert!(matches!( | ||
all_or_nothing::<String, _>( | ||
OptionalConfig::Empty("first.key"), | ||
OptionalConfig::Present { | ||
value: "test", | ||
key: "second.key" | ||
}, | ||
), | ||
Err(_) | ||
)) | ||
} | ||
} |
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.
If I'm understanding correctly, the function takes only two arguments, what would it look like if we wanted to use it with 3 or 4 arguments? Do we duplicate the function however many times we need or is there a way to solve this more generically, which we'll use when we need it?
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.
At the moment I'm trying to keep this bit simple as this is an area where we have a lot of scope for solving problems we don't really have. I will refactor this to a single tuple argument via a trait so we at least don't need to create a different
all_or_nothing
function for each cardinality, but for now, I'm going to avoid coming up with any particularly clever solutions to generalising this, as I'm not convinced we really have a use case.