Skip to content

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

Merged
waywardmonkeys merged 5 commits into
endoli:mainfrom
Hywan:feat-jsvalue-array
Oct 5, 2023
Merged

feat: Implement JSValue::new_array and other small clean up#17
waywardmonkeys merged 5 commits into
endoli:mainfrom
Hywan:feat-jsvalue-array

Conversation

@Hywan
Copy link
Copy Markdown
Contributor

@Hywan Hywan commented Oct 5, 2023 •
edited
Loading

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 Hywan mentioned this pull request Oct 5, 2023
Merged
Hywan added 5 commits October 5, 2023 14:22
@Hywan
doc: Format doc for consistency.
935ee91
@Hywan
feat: Implement JSValue::new_array.
2fd73f9 
This patch implements `JSValue::new_array`, which creates an array pre-
filled with items of kind `JSValue`.
@Hywan
feat: Implement JSValue::new_inner.
7f6c13e 
This patch implements a new constructor: `JSValue::new_inner` that
replaces a manual struct construction.
@Hywan
feat: Implement From<JSValue> for JSException.
8c2254a 
This patch implements `From<JSValue>` for `JSException`. It simplifies
the code a little bit.
@Hywan
doc: Add an example for JSValue::new_array.
ff58b54
@Hywan Hywan force-pushed the feat-jsvalue-array branch from 943a07e to ff58b54 Compare October 5, 2023 12:22
waywardmonkeys
waywardmonkeys approved these changes Oct 5, 2023
View reviewed changes
Copy link
Copy Markdown
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
Copy Markdown
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
Copy Markdown
Contributor

waywardmonkeys commented Oct 5, 2023

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
Copy Markdown
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?

@Hywan Hywan mentioned this pull request Oct 12, 2023
Closed
32 tasks
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Reviewers

@waywardmonkeys waywardmonkeys waywardmonkeys approved these changes

Assignees

No one assigned

Labels

None yet

Projects

None yet

Milestone

No milestone

Development

Successfully merging this pull request may close these issues.

2 participants

@Hywan @waywardmonkeys