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

json now defaults Option<_> to None. #16971

Merged
merged 1 commit into from
Sep 9, 2014
Merged

json now defaults Option<_> to None. #16971

merged 1 commit into from
Sep 9, 2014

Conversation

treeman
Copy link
Contributor

@treeman treeman commented Sep 3, 2014

Closes #12794.

match def.fields[index].node.ty.node {
TyPath(ref path, _, _) => {
match path.segments.as_slice().get(0) {
Some(x) => x.identifier == token::str_to_ident("Option"),
Copy link
Contributor

Choose a reason for hiding this comment

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

This is not going to work right if I have a type named Option that is not std::option::Option.

Similarly, it presumably will not work right if I rename Option, using either use std::option::Option as Foo or type Foo = std::option::Option<int>.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Good point, I totally missed that.

@lilyball
Copy link
Contributor

lilyball commented Sep 3, 2014

I feel like the right approach here really is to be able to provide a default value to use if the field isn't present. This is just another instance of the need to be able to customize deriving.

@alexcrichton
Copy link
Member

Note that it is currently possible for decoders to emulate this behavior today, as it's what toml-rs does (and used quite extensively in cargo).

@treeman
Copy link
Contributor Author

treeman commented Sep 4, 2014

I agree @kballard, we need default values.

Thanks @alexcrichton, will take a peek.

@treeman
Copy link
Contributor Author

treeman commented Sep 9, 2014

Reworked the Option handling and now defaults to None for json only.

#17089 may enable us to specify default arguments, something like:

#[deriving(Decodable)]
struct Data {
    #[default = None]
    x: Option<uint>,
}

I still think it's more ergonomic to have Option default to None, at least in the json case.

@treeman treeman changed the title Derived Decodable can now handle Option<_> as optional input. json can now defaults Option<_> to None. Sep 9, 2014
@treeman treeman changed the title json can now defaults Option<_> to None. json now defaults Option<_> to None. Sep 9, 2014
alexcrichton added a commit to alexcrichton/rust that referenced this pull request Sep 9, 2014
@bors bors merged commit 4f4a3df into rust-lang:master Sep 9, 2014
@treeman treeman deleted the json-decode branch September 10, 2014 05:16
bors added a commit to rust-lang-ci/rust that referenced this pull request Mar 31, 2024
Resolve tests per file instead of per crate in test explorer

Fix part of rust-lang#16827
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.

Modify Json Decoder to handle missing/optional Json fields if mapped to Option
4 participants