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

Change CWriter's Converter to an Assoc Type #134

Merged
merged 1 commit into from
Nov 27, 2024

Conversation

Pspritechologist
Copy link
Contributor

The CTypeConverter doesn't follow the same pattern as the CWriter (implementing a trait of mostly default fns) making it very difficult to alter the functionality of. In addition, CWriter itself uses a static Converter type, disallowing altering it at all.
This pr changes the Converter CWriter uses to an associated type, allowing any type to be used in implementations of CWriter, whether or not they change any other logic.

I was going to do the same for the Python and C# backends, but I noticed their converters didn't implement Traits at all, so I left them alone. I'm happy to do the same for them if this change is favourable.

This is a breaking change of course since implementations of CWriter will need to add the new associated type (something default assoc types would fix 😔).

@Pspritechologist
Copy link
Contributor Author

By the way, I was wondering if there was somewhere to discuss questions or thoughts about the library more casually than Issues. I didn't see any community chat linked to this project.
Would probably be a nice to have for something as deep as language bindings, especially since this project is designed to work very well with third party Crates.

@ralfbiedert
Copy link
Owner

Thanks! I'll look into this probably later tonight.

there was somewhere to discuss questions or thoughts about the library more casually than Issues

Right now "Discussions", but beyond that there's nothing else really.

@ralfbiedert
Copy link
Owner

Nm, just saw this is only a few lines.

@ralfbiedert ralfbiedert merged commit 937a45d into ralfbiedert:master Nov 27, 2024
2 checks passed
@Pspritechologist
Copy link
Contributor Author

Nm, just saw this is only a few lines.

Yeah, the diff is quite misleading :P
Just collapsing the trait def down onto the previous impl.

@Pspritechologist
Copy link
Contributor Author

Right now "Discussions"...

I forgot that existed!

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