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

Provide extra functions to return Options when the input is null as Serde usually does #4

Open
saikrishna1-bidgely opened this issue Dec 29, 2022 · 4 comments · May be fixed by #5
Open

Comments

@saikrishna1-bidgely
Copy link

  • serde-this-or-that version: 0.4
  • Rust Compiler (rustc) version: 1.66.0 (69f9c33d7 2022-12-12)
  • Operating System: Mac

Description

I want to get an Option when there is null value like Serde usually does. Currently, the exported functions only support types and not Options of those types.
I want some extra functions like as_opt_bool, as_opt_f64, as_opt_i64, as_opt_string, as_opt_u64 which will return an Option of the respective data type.

@corneliusroemer
Copy link

I agree this would be very useful and make code safer.

Turning an empty string into 0.0 is a strong assumption that may not at all be what the user wants and can lead to surprising bugs.

@rnag
Copy link
Owner

rnag commented Mar 17, 2023

Agreed! I just looked into this issue now, but I concur that having additional functions such as as_opt_bool could be a good idea. Its also good for backwards compatibility, so that it doesn't break or change existing logic.

In any case, I would welcome a PR to achieve these changes, else I can take a look into it when I have some time.

@corneliusroemer
Copy link

Sounds great @rnag

I've recommended serde-this-or-that in this SO answer: https://stackoverflow.com/a/75684771/7483211

Would be neat if it also covered the safer Option use case. Unfortunately I've got a lot on my plate at the moment. I'd be happy to review the feature, however, if you make progress!

@rnag rnag linked a pull request Mar 17, 2023 that will close this issue
@rnag
Copy link
Owner

rnag commented Mar 17, 2023

@saikrishna1-bidgely @corneliusroemer I've checked in code to a WIP branch and created a PR (#5) to incorporate the changes, please take a look and let me know any thoughts or feedback.

The main points to note here:

  • I basically copied de_impl.rs as a separate file, and mostly just wrapped all the return values into Some(value). For code de-duplication purposes, I'd be curious if I can wire together a macro for this purpose, i.e. to duplicate the Visitor class but wrap all return values so that it returns an Option instead.
  • I removed all cases where we return error, so E::invalid_value is never used, instead we return a None in this scenario.
  • In cases were parsing fails, for instance into u64 or f64, we just silently return a None value.
  • Updated logic for Visitor class - please confirm if this is acceptable:
    • as_opt_bool, string values that are neither "truthy" nor "falsy" de-serialize into None

TODOs:

  • Update docs for the new optional functions, and also update main docs in readme

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 a pull request may close this issue.

3 participants