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

Update dis_macro and add ref dis injection #19

Merged
merged 5 commits into from
Feb 26, 2024

Conversation

garethj2
Copy link
Contributor

Two changes here...

  1. dis_macro: now returns a Cow for the get_value function passed into it. This gives more flexibility when using the API. Please note, no version has been published since this last change so it can't be considered a breaking change.
  2. ref dis injection: I needed someone to inject Ref display name values into decoded objects. This is required in a haystack server when one is dynamically calculating a record's display value and needs to share it with all record's in the database in an efficient manner. I admit I'm not 100% happy with this. I couldn't find a nice efficient way to do some sort of configuration injection into a Serde deserializer. Regardless, I've isolated this to a specific part of the library so it doesn't pollute the core Ref value type. I think this really could be enhanced further by making the core API able to share references more easily and hence use proper interning for granular haystack data types.

@garethj2 garethj2 self-assigned this Feb 23, 2024
src/haystack/encoding/json/decode.rs Outdated Show resolved Hide resolved
src/haystack/encoding/json/decode.rs Outdated Show resolved Hide resolved
src/haystack/encoding/json/encode.rs Outdated Show resolved Hide resolved
@garethj2
Copy link
Contributor Author

Would appreciate your thoughts on this @rracariu . I just want to make sure you're happy with my ref display name injection addition.

Copy link
Contributor

@rracariu rracariu left a comment

Choose a reason for hiding this comment

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

I would go with a trait that implements the generic approach in the library, and then re-implement this trait in another crate using the new-type pattern, see Using the Newtype Pattern to Implement External Traits on External Types
IMHO this would make this less fragile and most likely more performant as there are less indirections and lock access, in exchange of a little more coding effort in the consumer crate.

To summarize: crate a trait for this macro facility, implement it for Value and Ref in this library crate, use this default implementation for the ser/de, then in another crate that depends on this library, create wrappers for Value and Ref (List, Dict, Grid?) implement the dis macro trait for these wrappers and partially implement the Serde traits for serialize/deserialize parts where you do your specialization for whatever use case you have, and delegate the rest to the default Value and Ref types. Create wrappers for zinc if this is the case.

If you want, you could completely circumvent the library type crate and only use the wrappers in the consuming crate by using transparent references via the Deref polymorphism
I don't think this would be need it, as probably you only need this facility of ser/de parts, so in this case those wrapper structs would suffice.

@garethj2
Copy link
Contributor Author

garethj2 commented Feb 26, 2024

I would go with a trait that implements the generic approach in the library, and then re-implement this trait in another crate using the new-type pattern, see Using the Newtype Pattern to Implement External Traits on External Types IMHO this would make this less fragile and most likely more performant as there are less indirections and lock access, in exchange of a little more coding effort in the consumer crate.

To summarize: crate a trait for this macro facility, implement it for Value and Ref in this library crate, use this default implementation for the ser/de, then in another crate that depends on this library, create wrappers for Value and Ref (List, Dict, Grid?) implement the dis macro trait for these wrappers and partially implement the Serde traits for serialize/deserialize parts where you do your specialization for whatever use case you have, and delegate the rest to the default Value and Ref types. Create wrappers for zinc if this is the case.

If you want, you could completely circumvent the library type crate and only use the wrappers in the consuming crate by using transparent references via the Deref polymorphism I don't think this would be need it, as probably you only need this facility of ser/de parts, so in this case those wrapper structs would suffice.

Thanks for the great advice @rracariu . I've implemented this and it works very nicely! I've removed the ref display name injection from the code base. I did make the serde visitor decoder public so I could reuse when I create my own serde decoder visitor. I agree this is a far better approach for now. It keeps this library nice and clean.

Copy link
Contributor

@rracariu rracariu left a comment

Choose a reason for hiding this comment

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

LGTM

@garethj2 garethj2 changed the title Update dis_macro and add ref dis injectiond Update dis_macro and add ref dis injection Feb 26, 2024
@garethj2 garethj2 merged commit df83432 into master Feb 26, 2024
8 checks passed
@garethj2 garethj2 deleted the feature/STK-2260-dis-macro-and-ref_dis-inject branch February 26, 2024 18:31
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.

3 participants