Skip to content

feat: Implement JSValue::new_array and other small clean up #17

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

Merged
merged 5 commits into from
Oct 5, 2023

Conversation

Hywan
Copy link
Contributor

@Hywan Hywan commented Oct 5, 2023

Built on top of #16.

This PR should ideally be reviewed commit-by-commit for a better experience.

Basically, it adds JSValue::new_array.
In addition, it adds JSValue::new_inner which simplify the code greatly.
Finally, it implements From<JSValue> for JSException, which adds another simplification of the code.

Hywan added 5 commits October 5, 2023 14:22
This patch implements `JSValue::new_array`, which creates an array pre-
filled with items of kind `JSValue`.
This patch implements a new constructor: `JSValue::new_inner` that
replaces a manual struct construction.
This patch implements `From<JSValue>` for `JSException`. It simplifies
the code a little bit.
@Hywan Hywan force-pushed the feat-jsvalue-array branch from 943a07e to ff58b54 Compare October 5, 2023 12:22
Copy link
Contributor

@waywardmonkeys waywardmonkeys left a comment

Choose a reason for hiding this comment

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

I'm tempted to just merge this as is, but I wanted to bring up some thoughts I had about JSException since this touches some of that.

I think what you have is fine for now, but should be revisited in that area comprehensively. I think the JSException should be storing a JSObject instead which is an actual exception. There's some other accessors that need to be added.

I was looking at https://github.com/WebKit/WebKit/blob/f8937a35f02f3c64eb02529b413b65c33d65e565/Source/JavaScriptCore/API/glib/JSCException.cpp for some ideas on what things should look like in this regard.

But as I said ... I think I'll land this and then we can revisit.

What do you think?

@Hywan
Copy link
Contributor Author

Hywan commented Oct 5, 2023

I would prefer to see this PR merged, and to open an issue to land the ideas about JSException improvements. I've other ideas about exceptions too, like result and exception must be exclusively set if I understand it well, and we should add some macros to handle and to ensure that. It can part of another PR.

@waywardmonkeys
Copy link
Contributor

Sure thing. And I was out for a couple of hours (I'm GMT+5), but I'll be around to merge stuff and so on for the next 3-4 hours now.

@waywardmonkeys waywardmonkeys merged commit c03aaa3 into endoli:main Oct 5, 2023
@Hywan
Copy link
Contributor Author

Hywan commented Oct 5, 2023

Perfect, thanks! I'm on UTC+2. Maybe we can talk in a Matrix room or a similar network?

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.

2 participants