Export as_opt_<type> helper functions
#5
Conversation
There was a problem hiding this comment.
There seems to be some differing assumptions on what should be None and what shouldn't.
I find the examples section hard to read. Rather than having many examples, it may be better to have unit tests - and examples (doc tests) in src/de_impl_opt.rs doc strings.
Somewhere there should be a dense table/section where the behaviour can be seen at a glance - possibly in the doc string itself. I like the # Returns section, explaining behaviour, but this could be complemented with more comprehensive cases for each of the input types str, i64, f64, u64.
|
|
||
| let m: Msg = serde_json::from_str(data).unwrap(); | ||
| trace!(" {m:?}"); | ||
| assert_eq!(m.timestamp, Some(1)); |
There was a problem hiding this comment.
I think this could arguably be None. To round or not to round...
There was a problem hiding this comment.
Agreed! When I searched online, I found that the rounding logic can be considerably more complex, as it relies on if a number to the left of the decimal point is even or odd. This makes rounding 0.5 ambiguous, as 0 is neither odd or even (or maybe both?).
For now, I wouldn't say that it's necessarily wrong to round 0.5 to 1. That is, it appears that the rounding logic is at least consistent, so that it is expected that 0.5 would round upwards to 1 (same with 3.5 or 4.5 for example).
However, I'm definitely open to exploring this scenario more in detail, and seeing what differing thoughts and opinions might arise on this issue with rounding numbers in this case.
…())` rather than `None`
Co-authored-by: Cornelius Roemer <cornelius.roemer@gmail.com>
…ool)`, and others to `None`
Hi @corneliusroemer ! Thanks for bringing this up. I totally agree, that at some point it would be worth it to refactor it to move example cases into doc tests & unit tests more explicitly, that way it also runs automatically in the GitHub Actions workflow (CI) and also when deploying new versions. I've created a new issue to track effort on this separately, as part of #7 . For now, as and when when time allows, please take a look at recent changes and commits I have added as part of this PR, to confirm if it resolves or addresses all issues based on your notes. I will hold off on merging the changes until then. Thanks, and have a great Monday 🎉 ! |
|
I just stumbled upon this crate and the first thing I wanted was |
|
@josephwynn-sc Yes, totally! Sorry for the long wait, just coming back after a hiatus. I'll look into it this weekend, run some tests with |
Closes #4