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

Value implementation for `Option<T> where T: Value #878

Closed
wants to merge 4 commits into from

Conversation

akshatagarwl
Copy link

@akshatagarwl akshatagarwl commented Aug 3, 2020

Closes #824

@akshatagarwl akshatagarwl marked this pull request as ready for review August 6, 2020 00:06
@akshatagarwl akshatagarwl requested review from hawkw and a team as code owners August 6, 2020 00:06
@akshatagarwl akshatagarwl changed the title [WIP] Value implementation for Option<T> where T: Value Value implementation for `Option<T> where T: Value Aug 6, 2020
/// Visit an Option
fn record_option(&mut self, field: &Field, value: Option<&(dyn Value + 'static)>) {
if let Some(inner_value) = value {
self.record_debug(field, &inner_value)
Copy link
Contributor

Choose a reason for hiding this comment

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

I don't think this implements the recording of the interior type using the appropriate type-specific record_* method as described in the linked task.

You probably want to call the record method on the inner_value.

Copy link
Member

Choose a reason for hiding this comment

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

Yup, that's right.

Copy link
Author

Choose a reason for hiding this comment

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

Is this right inner_value.record(field, self)?

Copy link
Member

Choose a reason for hiding this comment

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

Is this right inner_value.record(field, self)?

Yeah, that's right.

Copy link

@Michael-F-Bryan Michael-F-Bryan left a comment

Choose a reason for hiding this comment

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

Thanks for making this PR @humancalico.

I'd like to use it so I've made some suggestions that should hopefully make CI pass and unblock progress.

/// Visit an Option
fn record_option(&mut self, field: &Field, value: Option<&(dyn Value + 'static)>)
where
Self: std::marker::Sized,

Choose a reason for hiding this comment

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

You don't want to have a Self: Sized constraint here because this trait is meant to be used with dynamic dispatch.

If you added it because of the borrow checker try using a reborrow (&mut *self) to pass self to inner_value.record().

if let Some(inner_value) = value {
inner_value.record(field, self)
} else {
}

Choose a reason for hiding this comment

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

Can this else { } be removed if we aren't going to do anything with it? If a serializer cares about the lack of a value they can always override this method and do a match themselves.

@akshatagarwl
Copy link
Author

Thanks! @Michael-F-Bryan

@hawkw
Copy link
Member

hawkw commented Sep 24, 2021

Obsoleted by #1585, sorry about that!

@hawkw hawkw closed this Sep 24, 2021
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

Successfully merging this pull request may close these issues.

core: consider Value implementation for Option<T> where T: Value
4 participants