-
Notifications
You must be signed in to change notification settings - Fork 1.3k
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
Expose get_module_export
from Caller
#9525
Conversation
What I might recommend for your use case is to store a Alternatively if you want the absolute fastest route what I think you'd do is:
That front-loads as much as possible at the cost of safety, so I'd recommend using |
(although if you'd like adding the API like in this PR I also think is reasonable!) |
/// Returns `None` if there was no export with a matching entity index. | ||
/// # Panics | ||
/// | ||
/// Panics if `store` does not own this instance. |
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.
Docs are effectively a copy/paste of Instance::get_module_export
. Happy to take recommendations or if you want to make a suggestion, I will definitely accept it. I think see [Instance::get_module_export] for details
would probably also be fine so long as it's clear that the caller
has access to the instance (wasn't clear to me without looking at the code).
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.
Looks good to me! Mind adding a small smoke test too?
@alexcrichton not sure if this is what you meant by a smoke test, but it works as one, haha 😅 . |
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.
Thanks!
Not necessarily trying to get this merged, just using it as an example for my comment here