Derive TryIntoJs for structs #148
Conversation
|
Hi @mkawalec, thanks for the PR! I'll try to read through it and have some comments by the end of the day :) |
|
let me see if we can change |
|
|
||
| fn main() { | ||
|
|
||
| } |
There was a problem hiding this comment.
Parser doesn't support Enum and Union. That should be include in fail cases
|
@sehz Thank you. I didn't look into using |
|
I took a first-pass read through this today and everything looks really good, I think you did a great job. I wanted to play around with cargo-expand on some of the examples but for some reason it is not working for me, so I wasn't able to dig around with that like I was planning to. Tomorrow I'll take another deeper look and try to test the limits of this a bit more than the one test. Thanks again for putting in such great work! |
|
Any update? Just need add fail test case |
|
@sehz I haven't had any free time in the meantime, with a bit of luck this will happen this week. I want to get rid of the |
|
I think I've got it - now only references need to be |
|
@mkawalec I've just finished reading through this again and I think I'm happy with just about everything in here. The one question I have is why you decided to add support for unit structs? It seems to me that there isn't much use to converting those at all. It also took me awhile to figure out that they turned into empty objects. I was wondering if you could just talk me through why you decided to do it that way? |
|
My reasoning was purely consistency based - if structs translate to objects, a unit struct would translate to an object with no keys. I was mostly thinking about allowing complex structs with many levels of structs embedded inside of them to convert automagically into Node. That being said I am in no way set on that conversion - I can definitely see a merit of using a |
|
I suppose my initial reaction is that any of those options could make sense, but we currently have not seen a use-case that has demanded any approach over the others. With that being the case, I think I'd be more inclined to omit this functionality for now and wait to see whether anybody comes along who has a specific need for this and see what their requirements are. Would you mind removing the Unit struct support and perhaps we can open an issue to talk about its use case in the future? |
|
Sure, that's also a valid approach. I'll remove the unit struct support today and replace it with an error message telling the user to reach out if they need it. |
|
That sounds awesome, thanks! |
There was a problem hiding this comment.
Thanks for making those changes, I think everything looks good now! @sehz what do you think?
| @@ -34,12 +34,19 @@ impl TryIntoJs for i32 { | |||
| js_env.create_int32(self) | |||
| } | |||
| } | |||
|
|
|||
There was a problem hiding this comment.
Can you bump up version for nj-core?
|
rebase as well |
|
So the rebase went weirdly... It looks like it still does what it is supposed to do though, I highly recommend squash merge over rebasing for that exact reason. |
|
Great work. Thanks for contribution. |
This implements #147, without the bonus points.
I am fairly unhappy about the fact that we demand fields to be
Clone. Becausetry_into_jstakes ownership ofSelf, I am unsure how to run it for each of the struct's fields without cloning. Please do let me know if there is a way of making the borrow checker happy without cloning, it feels like there should be one.Added a
TryIntoJsinstance forusizeby casting it tou64. I don't think we expect this code to run on architectures wider than 64 bits, so the conversion should be a safe one.