cloudevents / sdk-rust

Rust library to interact with CloudEvents
Apache License 2.0
177 stars 61 forks source link

Proposal to allow destructuring of Event #243

Open ozabalaferrera opened 2 days ago

ozabalaferrera commented 2 days ago

I noticed this comment in the message tests: https://github.com/cloudevents/sdk-rust/blob/main/src/event/message.rs#L185

 #[test]
    fn message_v03_roundtrip_binary() -> Result<()> {
        //TODO this code smells because we're missing a proper way in the public APIs
        // to destructure an event and rebuild it

So I made some changes to support destructuring an Event: https://github.com/ozabalaferrera/cloudevents-sdk-rust/compare/oz_upgrade_all...ozabalaferrera:cloudevents-sdk-rust:oz_add_destructuring?expand=1

@jcrossley3, do you think this is a worthwhile change? I ran into situations in the past where I wanted to destructure, but didn't think much of it until I saw the test comments. I believe this change shouldn't break anything.

jcrossley3 commented 1 day ago

I'm sure I'm missing something, but what's the PhantomData for?

ozabalaferrera commented 1 day ago

I'm sure I'm missing something, but what's the PhantomData for?

I needed a zero-sized type for the private field. I'm used to PhantomData from doing type-state stuff, but the unit type () alone works just as well. Thanks for questioning that; I updated my repo's branch.

jcrossley3 commented 18 hours ago

I guess I still don't understand. If you remove the _private field, what happens?

ozabalaferrera commented 16 hours ago

Oh, I misunderstood your question, sorry. I added pub(crate) _private so that the struct has a private (outside the crate) field to prevent direct, field initialization of the struct (I forgot the term for this). The other fields can then be public to allow destructuring. This retains the public API, forcing the use of existing methods for creating the struct.

jcrossley3 commented 14 hours ago

Ah, now I understand. For me, the value of preventing direct, field initialization of the struct isn't worth the "type noise" of requiring the double-dot operator to destructure the Event. To borrow a Python term, I'm in the "consenting adults" camp. I'm ok making the fields public if it simplifies the calling code.

ozabalaferrera commented 11 hours ago

I figured those fields were private very intentionally since there are builders to ensure the constructed Event is valid per the CloudEvent specification. Maybe @Lazzaretti can weigh in.