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

feat(convert): add derive macros for To and FromBytes #667

Merged
merged 8 commits into from
Feb 5, 2024

Conversation

ModProg
Copy link
Contributor

@ModProg ModProg commented Jan 27, 2024

closes #661.

convert-macros/src/lib.rs Outdated Show resolved Hide resolved
@ModProg ModProg force-pushed the convert-derive-macros branch 3 times, most recently from cc4e9db to b45b57a Compare January 29, 2024 19:43
Copy link
Contributor

@zshipko zshipko left a comment

Choose a reason for hiding this comment

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

Looks great - I have a commit where I changed some of the tests to use this and it makes the developer experience very nice! Those changes are here: https://github.com/extism/extism/tree/test-convert-derive-macros

I'm not 100% clear how the right way to do the path resolution in this case, but I think your solution works pretty well - as long as the end user doesn't need to add any additional features when using the extism or extism-pdk crates.

I think we just need some more tests and documentation (like you mentioned in your issue) and it should be ready!

convert-macros/src/lib.rs Outdated Show resolved Hide resolved
@ModProg
Copy link
Contributor Author

ModProg commented Jan 29, 2024

as long as the end user doesn't need to add any additional features when using the extism or extism-pdk crates

Yeah, that is the idea. This would need to be extended for any “entry point” crates that are created in the future. But sadly, rust still doesn't have a $crate for proc macros.

@zshipko
Copy link
Contributor

zshipko commented Feb 2, 2024

Hey @ModProg - how's this going? If you're stuck on the documentation/testing I can help pick some of that up next week.

convert-macros/Cargo.toml Outdated Show resolved Hide resolved
convert/Cargo.toml Outdated Show resolved Hide resolved
Comment on lines +8 to +10
// Makes proc-macros able to resolve `::extism_convert` correctly
extern crate self as extism_convert;

Copy link
Contributor Author

Choose a reason for hiding this comment

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

These will only be relevant as soon as proc macros are used inside this crate. So not yet. But this will avoid any future confusion.

runtime/Cargo.toml Outdated Show resolved Hide resolved
Comment on lines +9 to 10
#[doc(inline)]
pub use extism_convert as convert;
Copy link
Contributor Author

Choose a reason for hiding this comment

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

This makes the convert crate show up as a module instead of a reexport. This makes the paths inside the examples correct.

@ModProg ModProg requested a review from zshipko February 3, 2024 15:33
@ModProg
Copy link
Contributor Author

ModProg commented Feb 3, 2024

Hey @ModProg - how's this going? If you're stuck on the documentation/testing I can help pick some of that up next week.

I added some, but feel free to expand on them.

Comment on lines +24 to +25
# Only required for tests run from the workspace root, as that enables the `extism-path` feature.
extism.workspace = true
Copy link
Contributor Author

Choose a reason for hiding this comment

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

Alternatively, we could add a hidden # mod extism {pub use extism_convert as convert;} to the doc tests.

@ModProg
Copy link
Contributor Author

ModProg commented Feb 3, 2024

The last two commits feel a bit like hacks to make ci happy :D

@zshipko
Copy link
Contributor

zshipko commented Feb 5, 2024

The last two commits feel a bit like hacks to make ci happy :D

Ah yeah the path resolution features seem to add some additional complexity around docs/testing but it seems worth it.

I think these changes looks great and appreciate the effort updating the docs! I will merge this later after I experiment with the changes a little more.

@zshipko zshipko merged commit 1a083f6 into extism:main Feb 5, 2024
5 checks passed
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.

derive that forwards From/ToBytes to specific encoding
2 participants