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

Extract proc macro impl into own crate #364

Merged
merged 1 commit into from
Nov 1, 2024

Conversation

shahn
Copy link
Contributor

@shahn shahn commented Oct 30, 2024

This helps testability of the proc macro, allows fuzzing it to find panics, and would also allow benchmarking.

This might also help investigations like in #362 and #363 to see if there are actual performance changes to the derive macros.

Careful review is probably necessary, because a new crate is introduced. I am happy to take this into any direction, but it is also the foundation for the error message improvements for the derive macros I am working on.

@shahn shahn changed the title Extract proc macro into own crate Extract proc macro impl into own crate Oct 30, 2024
@shahn
Copy link
Contributor Author

shahn commented Oct 30, 2024

I guess another idea would be to do it like serde does it, using a non-published crate that has a symbolic link for the actual implementation, to use the non-published crate for tests etc and publish the rest like normal. I guess that would be preferable, here, too. I will follow up with a commit to add that

@shahn shahn marked this pull request as draft October 30, 2024 03:22
@shahn
Copy link
Contributor Author

shahn commented Oct 30, 2024

Hrm, that has the downside of Windows (and serde actually publishes the serde_derive_internals crate, too). Do you have any preference?

@shahn shahn marked this pull request as ready for review October 30, 2024 11:11
This helps testability of the proc macro, allows fuzzing it to find
panics, and would also allow benchmarking.
@XAMPPRocky
Copy link
Collaborator

XAMPPRocky commented Nov 1, 2024

Thank you for your PR! We can just go with a separate crate for now.

@XAMPPRocky XAMPPRocky merged commit db612fc into librasn:main Nov 1, 2024
65 checks passed
This was referenced Nov 1, 2024
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