fix: partial support flatten enum in struct #15
Conversation
There was a problem hiding this comment.
I put some comments on the code. But I have some overall notes.
First of all, I'm a little uneasy with the change. There are at least 3 comments about hacks in this change, which kinda makes me uncomfortable. But I'm a lenient person, so I'm okay giving all of them a pass. I'm just uncomfortable because I'm not fully mastering this topic, or even the bug it's trying to fix.
However, this doesn't break backward compatibility. So I'm strongly considering approving this change.
Even though I don't know yet, there are some things that need to be addressed if you want me be able to merge it.
- Please make only one commit. I don't mind multiple commits on changes. But I hate clean up commits. I know this repo has many from me, but as the owner of the repo, I have the right to break my own rules 😄 . I just don't like it when other don't follow my rules 😛 . For this change, I would like to ask you to make only one commit please, because I think it fits nicely into one.
- Please rebase onto the
masterbranch, as I've said in one of the comments, the tests were failing when you forked it, and I fixed them ~1h ago. I want the CI to be green before merging this change. It is currently green onmaster, if it becomes red again, no need to fix it, I'll fix it again. We're regularly running master against the latest clippy, so when clippy adds new checks, it breaks once in a while. - Please use
git commit -sas mentionned in the README. This means that you signed off the Developer Certificate of Origin. See more information in my comment about it
Thank you for your patch, with a nice test 🙂 .
greenhat616
force-pushed
the
master
branch
2 times, most recently
from
4037163 to
35bb238
Compare
|
I have revised the comments and squashed the commits into a single signed commit. |
greenhat616
force-pushed
the
master
branch
2 times, most recently
from
ec3a7c0 to
39cfdee
Compare
|
Hi. I have updated the adviced lines. |
There was a problem hiding this comment.
I accidentally send two comments without starting review.
My main concern here that this changes is not described in the code, how they are worked and why in that way.
I understood that you've represent tagged YAML values as maps and use visit_map when deserializing value is serde's Content, but it is not quite clear why it is needed to call visit_map for that? Why visit_enum does not work or why we cannot use visit_map in all cases?
|
Hi, I’ve got a bit of a breather from my busy work, and over the next few days, I’ll start addressing these reviews. |
Close acatton#14. I adapted the implementation from [ron-rs/ron#451](ron-rs/ron#451). This is a workaround for Serde's internal buffer type used when deserializing via `visit_enum`. Signed-off-by: Jonson Petard <41122242+greenhat616@users.noreply.github.com>
Close #14
The coding approach is according to ron-rs/ron#451.
The
serde_yaml_ng::from_str::<Outer>(yaml).is_err()should always failed withinvalid type: unit value, expected a string, which is blocked by serde-rs/serde#1183