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

Add functionality to convert custom types #1149

Open
wants to merge 2 commits into
base: master
Choose a base branch
from

Conversation

lecafard
Copy link

Add some functionality to convert custom types, as well as allowing data to be injected during the conversion process.

Also reworked the registry into classes to reduce duplication, keeping in mind backwards compatibility.

@lecafard lecafard force-pushed the master branch 5 times, most recently from 74d31a4 to 306101c Compare February 1, 2025 13:47
@sinclairzx81
Copy link
Owner

@lecafard Hi, thanks for the PR. This one looks interesting (and sorry for the delay)

I'll need some time to sit down and properly review these updates (I am bit pressed with other projects atm), but did have a quick question regarding the rationale behind the additional data property to the Convert signatures.

I guess my initial thoughts is that metadata should be encoded and received via Schema only, but curious to know the thinking there.


The registry updates look good here (that's some great thinking). I'll need a few extra days to properly sit down and go through these updates, but at a high level, these seem good. Just let me know your thoughts on the additional data parameter, and can pick things up in a few days.

Cheers
S

@lecafard
Copy link
Author

lecafard commented Feb 2, 2025

Hi there, I originally added the data parameter as I had a use case where I needed to inject a specific encryption key that would be unique to the object being converted and couldn't think of another way to do that.

If there's a better approach, please let me know. I understand this is a rather niche use case, which might not be a good fit for this library.

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.

2 participants