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

Add support for the reviver function to JSON.parse #410

Merged
merged 11 commits into from
Jun 1, 2020

Conversation

abhijeetbhagat
Copy link
Contributor

@abhijeetbhagat abhijeetbhagat commented May 22, 2020

This Pull Request tries to fix #344.

This PR needs lots of changes to the way we extract object while recursing. I tried using match on the Value using as_object() but this results in a borrow problem since i am passing a &mut Value to walk. So at the moment, i am cloning the unwrapped object after checking if it is safe to do so.

Need feedback on that.

@Razican
Copy link
Member

Razican commented May 22, 2020

Thanks for the contribution! This will collide with #402, though. We should probably wait for that to land.

@abhijeetbhagat
Copy link
Contributor Author

@Razican Sure! Meanwhile, can you please take a look at the changes?

@Razican
Copy link
Member

Razican commented May 22, 2020

@Razican Sure! Meanwhile, can you please take a look at the changes?

Yep, I will check this today :)

Copy link
Member

@HalidOdat HalidOdat left a comment

Choose a reason for hiding this comment

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

This looks good! Check my comments there mostly just for readability and clippy issues.

boa/src/builtins/json/mod.rs Outdated Show resolved Hide resolved
boa/src/builtins/json/mod.rs Outdated Show resolved Hide resolved
@HalidOdat HalidOdat added the enhancement New feature or request label May 22, 2020
@HalidOdat HalidOdat added this to the v0.9.0 milestone May 22, 2020
@HalidOdat
Copy link
Member

HalidOdat commented May 23, 2020

The next thing we need to do is adding tests, so we insure it does not break in the future.

@abhijeetbhagat
Copy link
Contributor Author

Yes, working on it.

@abhijeetbhagat
Copy link
Contributor Author

@Razican @HalidOdat review?

Copy link
Member

@HalidOdat HalidOdat left a comment

Choose a reason for hiding this comment

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

This looks almost ready to merge :)

boa/src/builtins/json/mod.rs Outdated Show resolved Hide resolved
boa/src/builtins/json/tests.rs Outdated Show resolved Hide resolved
@abhijeetbhagat
Copy link
Contributor Author

@HalidOdat review?

Copy link
Member

@Razican Razican left a comment

Choose a reason for hiding this comment

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

This is good to go!

@Razican Razican merged commit 8255c3a into boa-dev:master Jun 1, 2020
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
enhancement New feature or request
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Implement the optional reviver parameter in JSON.parse( text [, reviver] )
3 participants