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

Register [[.rust_class just as $.rust_class #359

Merged
merged 6 commits into from
Jan 23, 2022

Conversation

shrektan
Copy link
Contributor

Hi,

I believe it's necessary to register [[ S3 method for exported Rust Class as well.

The reason is that, only so, we can call the method dynamically, e.g., method <- "method_abc"; RustClass[[method]]().

This is not possible for $.

Thanks.

@yutannihilation
Copy link
Contributor

Sounds good to me.

@andy-thomason
Copy link
Contributor

This is great. Good to update the Change log, too.

We may export trait methods to s3 methods in the future in which case we may optionally overload [[ as Index, but we
can cross that bridge when we come to it.

@shrektan
Copy link
Contributor Author

Sorry that I didn't notice there's a test package that needs to be checked. Now it should be ok.

@yutannihilation
Copy link
Contributor

Thanks for updating. One more thing, could you add a small test to verify x[["foo"]]() works as the same as x$foo() around here?

@shrektan
Copy link
Contributor Author

@yutannihilation added.

Copy link
Contributor

@yutannihilation yutannihilation left a comment

Choose a reason for hiding this comment

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

Looks good, thanks! I'll wait for a while before merging to see if there's any concern from other maintainers.

@yutannihilation
Copy link
Contributor

I'm merging now. Thanks for the contribution!

@yutannihilation yutannihilation merged commit 854e4ff into extendr:master Jan 23, 2022
@shrektan shrektan deleted the export_double_bracket branch January 23, 2022 07:55
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.

None yet

3 participants