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.

Sign up for GitHub

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

Jump to bottom

Support None option values. #20736

Merged
merged 1 commit into from
Apr 2, 2024
Merged

Support None option values. #20736

Changes from all commits
Commits
Show all changes
1 commit
Select commit
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
78 changes: 67 additions & 11 deletions src/rust/engine/options/src/lib.rs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -200,6 +200,23 @@ pub struct OptionValue<T> {
pub value: T,
}

#[derive(Debug)]
pub struct OptionalOptionValue<T> {
pub derivation: Option<Vec<(Source, T)>>,
pub source: Source,
pub value: Option<T>,
Copy link
Contributor

Choose a reason for hiding this comment

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

It looks like this is almost identical to OptionValue<Option<T>> (except derivation becomes Option<Vec<(Source, Option<T>)>); what do you think about using that instead?

I think we can still add this convenience method, as I believe this should work:

impl<T> OptionValue<Option<T>> {
    fn unwrap(self) -> OptionValue<T> { ... }
}

Alternatively, add combinator methods like fn map<U>(self, f: FnOnce(T) -> U) -> OptionValue<U> to OptionValue<T>, then the places that need unwrap can write some_option_value.map(|v| v.unwrap()) or some_option_value.map(Option::unwrap).

Copy link
Contributor Author

Choose a reason for hiding this comment

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

We could. That type for a derivation would be substantial overkill, since only defaults can be None. But I'm not sure having both types is a big problem? They are lightweight containers with no associated logic.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Thanks for the review. Will merge as-is for now, and consider simplifying this once the Python bindings are done and we can see how it all fits together.

Copy link
Contributor

Choose a reason for hiding this comment

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

Yep, not a problem at all, just exploring the solution space. Happy as is!

}

impl<T> OptionalOptionValue<T> {
fn unwrap(self) -> OptionValue<T> {
OptionValue {
derivation: self.derivation,
source: self.source,
value: self.value.unwrap(),
}
}
}

#[derive(Debug)]
pub struct ListOptionValue<T> {
pub derivation: Option<Vec<(Source, Vec<ListEdit<T>>)>>,
Expand Down Expand Up @@ -357,12 +374,15 @@ impl OptionParser {
fn parse_scalar<T: ToOwned + ?Sized>(
&self,
id: &OptionId,
default: &T,
default: Option<&T>,
getter: fn(&Rc<dyn OptionsSource>, &OptionId) -> Result<Option<T::Owned>, String>,
) -> Result<OptionValue<T::Owned>, String> {
) -> Result<OptionalOptionValue<T::Owned>, String> {
let mut derivation = None;
if self.include_derivation {
let mut derivations = vec![(Source::Default, default.to_owned())];
let mut derivations = vec![];
if let Some(def) = default {
derivations.push((Source::Default, def.to_owned()));
}
for (source_type, source) in self.sources.iter() {
if let Some(val) = getter(source, id)? {
derivations.push((source_type.clone(), val));
Expand All @@ -372,38 +392,74 @@ impl OptionParser {
}
for (source_type, source) in self.sources.iter().rev() {
if let Some(value) = getter(source, id)? {
return Ok(OptionValue {
return Ok(OptionalOptionValue {
derivation,
source: source_type.clone(),
value,
value: Some(value),
});
}
}
Ok(OptionValue {
Ok(OptionalOptionValue {
derivation,
source: Source::Default,
value: default.to_owned(),
value: default.map(|x| x.to_owned()),
})
}

pub fn parse_bool_optional(
&self,
id: &OptionId,
default: Option<bool>,
) -> Result<OptionalOptionValue<bool>, String> {
self.parse_scalar(id, default.as_ref(), |source, id| source.get_bool(id))
}

pub fn parse_int_optional(
&self,
id: &OptionId,
default: Option<i64>,
) -> Result<OptionalOptionValue<i64>, String> {
self.parse_scalar(id, default.as_ref(), |source, id| source.get_int(id))
}

pub fn parse_float_optional(
&self,
id: &OptionId,
default: Option<f64>,
) -> Result<OptionalOptionValue<f64>, String> {
self.parse_scalar(id, default.as_ref(), |source, id| source.get_float(id))
}

pub fn parse_string_optional(
&self,
id: &OptionId,
default: Option<&str>,
) -> Result<OptionalOptionValue<String>, String> {
self.parse_scalar(id, default, |source, id| source.get_string(id))
}

pub fn parse_bool(&self, id: &OptionId, default: bool) -> Result<OptionValue<bool>, String> {
self.parse_scalar(id, &default, |source, id| source.get_bool(id))
self.parse_bool_optional(id, Some(default))
.map(OptionalOptionValue::unwrap)
}

pub fn parse_int(&self, id: &OptionId, default: i64) -> Result<OptionValue<i64>, String> {
self.parse_scalar(id, &default, |source, id| source.get_int(id))
self.parse_int_optional(id, Some(default))
.map(OptionalOptionValue::unwrap)
}

pub fn parse_float(&self, id: &OptionId, default: f64) -> Result<OptionValue<f64>, String> {
self.parse_scalar(id, &default, |source, id| source.get_float(id))
self.parse_float_optional(id, Some(default))
.map(OptionalOptionValue::unwrap)
}

pub fn parse_string(
&self,
id: &OptionId,
default: &str,
) -> Result<OptionValue<String>, String> {
self.parse_scalar(id, default, |source, id| source.get_string(id))
self.parse_string_optional(id, Some(default))
.map(OptionalOptionValue::unwrap)
}

#[allow(clippy::type_complexity)]
Expand Down
Loading