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

YSON Fixes #1170

Merged
merged 5 commits into from
Dec 8, 2021
Merged

YSON Fixes #1170

merged 5 commits into from
Dec 8, 2021

Conversation

Emad-salah
Copy link
Contributor

Hey there!

This PR fixes this issue #1155, I've rewritten the YSON functionality and made it use a modified version of yieldable-json so it produces very similar output as to how the previous YSON implementation used to so values that are of type "function" are output as null in the JSON string, etc...

I've also run some tests both in our project and the Mocha tests that are under the /test folder and it seems like the new YSON implementation is fully backward-compatible with the existing code that utilizes it :)

Let me know if you have any questions or if anything needs to be modified!

Thanks!

@amark
Copy link
Owner

amark commented Dec 3, 2021

@Emad-salah wowow awesome!

Will pull. Would you be willing to add this as a file rather than overwrite, like lib/ison.js? And of course include the MIT license at at top (GUN is too, but just in case), or whatever license it is and maybe backlink/reference. Then just update lib/index.js & lib/radisk.js to require("./ison.js") instead of ./yson.js

@Emad-salah
Copy link
Contributor Author

Great, I've added the plugin's license at the top of the file and moved it over to lib/ison.js, I've also updated lib/server.js, lib/radisk.js, lib/stats.js, and test/common.js to use ison.js instead of yson.js!

Let me know if anything else needs fixing and I'll get to it ASAP! 😄

@amark amark merged commit 77162fc into amark:master Dec 8, 2021
@amark
Copy link
Owner

amark commented Dec 8, 2021

Perfect!

Oh wow, you replied same day as my comment. I'm so sorry, I get delayed a lot and github doesn't send me email notifications anymore. Thank you for your responsiveness, sorry I didn't see it.

Pulling. 👏 ⚡ 🔥 ⭐ 👏 ⚡ 🔥 ⭐ 👏 ⚡ 🔥 ⭐ 👏 ⚡ 🔥 ⭐ !

@Emad-salah
Copy link
Contributor Author

No worries, thank you for merging it! 😄

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