-
Notifications
You must be signed in to change notification settings - Fork 89
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: add impl-level exporting QoL macro #707
base: main
Are you sure you want to change the base?
Conversation
If I haven't missed anything, the PR should be ready for review whenever you've got time :) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Hi!
I'm not entirely convinced this pattern is preferable as it stands. The function metadata macros are already complex as it is, and adding another layer should be well motivated. I'll have to think about it some more.
To note on the current implementation, you should probably prefer having it implement a trait on the type being implemented (like InstallWith
) rather than the bare exporter
and list
functions. You should prefer using an approach where module installer functions should belong to the module rather than the type being exported, since this is the pattern for installing everything else.
Since it is a type, maybe something like this:
let mut module = /* module being declared */;
module.ty_with_exports::<T>();
module.ty::<T>().with_exports();
That either which requires the Exports
trait or whatever you opt to call it to be implemented and the appropriate trait methods to install all exported items will be called.
let docs = reparsed.docs; | ||
let arguments = reparsed.arguments; | ||
let meta_kind = syn::Ident::new( | ||
["function", "instance"][reparsed.takes_self as usize], |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This is a bit too clever, prefer just using an if statement instead.
@@ -77,6 +78,54 @@ pub fn function( | |||
output.into() | |||
} | |||
|
|||
/// Create a function to export all functions marked with the `#[rune(export)]` attribute within a module. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
within an impl block, not a module?
Hi! Thank you for the review and sorry for the delay. The main motivation for grouping things like this is to avoid the need to have a human error-prone process when altering a type's API as you jump between definition and export locations. Most projects will likely have pretty simple export configurations so this method also streamlines the experience when trying out the library for example, leaving the specifics for later (e.g. I originally expected Though using a trait would be much cleaner, it'd make it impossible to export multiple impl blocks using this method; I don't know how much of a problem that is as I usually don't split my impls but I don't have any data on how common/useful that practice is to others. I'll make those changes within the a couple of days if/when I get your go on this :) |
This adds a new macro that allows for more concise exporting of a lot of functions.
Currently in a sort of PoC state, a lot can and will be improved.
Why ?
Having to edit multiple places when making simple changes to an API is tedious and a source of error.
How ?
Roadmap
I had to add the
extra-traits
syn feature, if you know of a way to do the comparison oncrates/rune-macros/src/item_impl.rs:70
without it that'd be pretty cool.