search

joshgoebel / wren-essentials

The good stuff you'll need that you can't find in the CLI
MIT License
5 stars 3 forks source link

Json support? #2

Closed clsource closed 3 years ago

clsource commented 3 years ago

I think JSON encode/decode can be a useful tool. A similar API and implementation can be used from DOME

https://github.com/domeengine/dome/blob/main/src/modules/json.wren

joshgoebel commented 3 years ago

I'm not sure about just gobbling up/duplicating things that other libraries already do well:

Now if the question is should we perhaps vendor and compile those other libraries as part of essentials that would be an interesting discussion... but then we're getting into how packaging management should work... really perhaps essentials just adds wren-json to it's package.wren file and then when you install dependencies you get wren-json as part of things... we could perhaps wrap it (or just provide an import/export for conveience).

import "essentials" for JSON // imports wren-json from packages
joshgoebel commented 3 years ago

A similar API and implementation can be used from DOME

Do you have any idea of pros/cons vs the existing wren-json implementation?

clsource commented 3 years ago

When evaluating wren-json for DOME it wasnt selected mainly due to

So it can be used as long as you are OK with delegating the maintainability and API design to third parties.

IMHO I prefer cohesive libs. So wren essentials can be a cohesive unit :)

joshgoebel commented 3 years ago

Perfomance dealing with JSON files

This is for sure a consideration - have we seen if we can upstream those fixes into wren-json though? The library author seemed quite willing to get my fixes for character encoding added...

as long as you are OK with delegating the maintainability and API design to third parties.

We don't have to delegate both. We can wrap poorly designed APIs... and delegating the maintainability can actually be a BIG win, provided there is a maintainer who is serious about their project long-term (vs a write and forget).

I've learned a lot since I've become maintainer of Highlight.js. Owning the world sounds nice until you find out how much effort it takes to maintain it all.

@brandly any thoughts on this?

joshgoebel commented 3 years ago

Does DOME use JSON for a lot of data/configs and such things?

clsource commented 3 years ago

normally they are used for game level and other game related data. But not for the core engine as far as I know, just as a tool for game devs.

joshgoebel commented 3 years ago

@clsource When you said it was faster I wasn't imagining you were simply wrapping an external lib... you should have said that outright I think. :-) I think wren-json is pure Wren. :-) Obviously there are real benefits to wrapping existing libraries - less code to maintain, just to name one benefit - not to mention the speed.

So that could easily tip me in favor of your JSON implementation.

So my next question would be:

That way you're still the official maintainer of record - yet the rest of the ecosystem could easily pull in updates as JSON moves forward.

Thoughts?

CC @avivbeeri

avivbeeri commented 3 years ago

I'm not sure exactly what kind of input you want from me here, but for context, DOME doesn't have any kind of package system. The json module, like all DOME's modules, is baked in to the engine. This isn't likely to change in the foreseeable future.

joshgoebel commented 3 years ago

This isn't likely to change in the foreseeable future.

Ok. thanks.

@clsource Ok... so as the maintainer of the json module what I'd love to propose:

IE the process I'd love to see for Essentials:

And upgrading the module would involve:

Then you might have a tiny script that copies the critical pieces of wren-fastjson into DOME (for continued maintainence of it's own vendored copy)?

Thoughts?

clsource commented 3 years ago

I think DOME have it's own set of features that are tailored to the engine. So separating that dependency it would be messy. This can be a package tailored for wren-console, just inspired in the DOME implementation. 👍

joshgoebel commented 3 years ago

The part I'm worried about is doing all this by hand... and then have no canonical copy of JSON (to track issues, bugs, etc)... making it impossible to know which version of the package/library Essentials is using, or DOME is using, etc...

joshgoebel commented 3 years ago

This can be a package tailored for wren-console

Well if you made a repo I'd imagine it would have some easy way to copy the files into DOME... I'm assuming you maintain the copy there? I'm guess that's just a tiny Bash script that copies two files perhaps - so that's the easy part. Or maybe you're not even worried about doing it by hand it's so easy :-)

joshgoebel commented 3 years ago

This can be a package tailored for wren-console

I'd love if it was just a package package, but we don't have all the pieces fully in place yet for binary libraries, which sadly means it has to be baked into wren-console... (so you'd have to install/vendor the package into our source tree, then compile, etc)...

clsource commented 3 years ago

I don't know if DOME would like to use a separated dependency for it's json encoder/decoder since its coupled with the FileSystem class and C bindings

Example:

The best is just to create an independent package and see if in the future DOME would like to use it, depending on Avivbeeri's leadership.

joshgoebel commented 3 years ago

its coupled with the FileSystem class

Ugh I hadn't even thought of that... :-) Ugh, maybe me write once, share everyone was a pipe dream.

clsource commented 3 years ago

Wren is an emerging ecosystem so its expected to have some rough edges :) I hope someday (not so far in the future) there would be more standarization between projects 👍

For now a simple json module inside wren-essentials can be done to supply json needs 💯

joshgoebel commented 3 years ago

Ok, lets go for it. :-) I'd still like to preferably version it somehow (I think that would be to your benefit, even if the code is slightly diff across projects - thoughts?)... JSON.packageVersion or something... and then wren-console would likely expose that via it's Runtime API... but that's a different PR.

So I'm imagining:

Does that sound about right? :-) Then I'll figure out if we want to vendor it permanently (or fetch it during build) and do that part of it, then bump versions, then bump wren-console and we'll have JSON. Yippy!

clsource commented 3 years ago

I think is best to version wren-essentials as a whole unit instead of a version of a single module. It would ease the version management in wren-console.

Git submodules works well enough in my opinion. The vendor thing would be needed only for the deps that does not have a git repo.

But if the dep is just a single file or just C and H headers then copy pasting would be enough.

joshgoebel commented 3 years ago

Git submodules works well enough in my opinion.

I've heard horror stories and never used them a lot, so we'll see. :-) I definitely don't find them as intuitive as the rest of git (and I know a lot of git) and i've run into caes where I had to hack my .git folder to back out weird situations befor...

I think is best to version wren-essentials as a whole unit

Oh I agree, that's not going away... but if you have a library that is 90% shared code between DOME, Wren-Console, other Wren project don't you think for your own purpose (and bug tracking) it would be useful to ay that the version of JSON bundled with DOME is 1.9 and the version bundled with wren-essentials is 2.0?

I mean perhaps we're too early for that yet, but it seems clear that as a maintainer you might want that - I certainly would.

joshgoebel commented 3 years ago

But if the dep is just a single file or just C and H headers then copy pasting would be enough.

The fewer files the more open I am to vendoring, even though it's really a shell script kind thing ether way.

clsource commented 3 years ago

As far as I know wren-essentials main deps are wren and libuv if git submodules are cumbersome then I think is fine to those two deps be vendored in.

Wren has an amalgamated file that can be used instead of the whole directory :).

For Wren Only deps like wren-assert and wren-testie then those should be inside a modules directory since they are part of the core, and does not have huge amount of files.

For wren modules and its files then a directory can be used 👍 

src/
  modules/
    mirror/
        mirror.c
        mirror.h
        mirror.wren
        mirror.wren.inc
joshgoebel commented 3 years ago

Wren has an amalgamated file that can be used instead of the whole directory :).

Except I had to patch some of the source for our unique eval capabilities, so it's probably easier to keep using the Wren source tree for now (since it's already vendored) and libuv is also... I did it cause it was easy and I wanted to focus on the other moving pieces.

wren-assert

This is a dev-only dependency (and we will probably switch to wren-testie long-term, which bundles it's own expectation lib). So I think it can just be installed manually.

wren-testie

Testie isn't currently part of Essentials, are you saying it should be? It's currently not even shipped with wren-console... wren-package is shipped just to make the getting start curve easier for newbies.

mirror.wren.inc

I don't believe in 100 of these files, which is why I generate a single combined .inc... and that's how I plan to keep it unless someone has really good arguments otherwise. I'm not even 100% sure they should be in source control at all, but for now I see the expediency.

src/mirror/mirror./*

I'm not sold that modules that are only 2-3 files need a subfolder, so lets table that for later... or if you want to try and show me the way, then do that in your own JSON PR (src/json/*), but please don't move any of the other files around. :-)

brandly commented 3 years ago

Hey, sorry I'm late! Sounds like you've already figured out a plan, but I'll share some thoughts.

wren-json aimed to be a nice parser that would parse plain strings. Really, I read Crafting Interpreters and just wanted to write a parser.

I'm guessing performance concerns are largely due to the fact that wren-json takes a string instead of streaming a file. Potentially there are parsing primitives that could be expose such that a file-streaming parser could be built on top of it. I would hesitate to tie it to the file system or to some non-standard stream primitive though.

I'll also mention that parsing JSON is a minefield, so anything we can do to avoid bringing more JSON parsers into the world should be considered!!

I'm pretty out of the loop with Wren these days, but let me know if I can be helpful!

p.s. What's the latest with modules/packages these days? Years ago, I started to implement a "package manager" on top of git, but the language wasn't really ready yet.

joshgoebel commented 3 years ago

Really, I read Crafting Interpreters and just wanted to write a parser.

Great book!

I'm guessing performance concerns are largely due to the fact that wren-json takes a string instead of streaming a file.

You think? Instead of the fact that it's pure Wren vs Wren/C?

so anything we can do to avoid bringing more JSON parsers into the world should be considered!!

Probably a fair thought. Since it already exists in DOME it's not exactly "more" though... also it's based on pdjson, so it's not even a pure re-implementation from scratch - rather just building on the C library for foundation.

but let me know if I can be helpful!

Your test suite was actually super helpful. I just plugged in your test suite almost without changes and we're passing all but a single test. (the errors messages aren't the same, but we still flag the JSON errors themselves, which is what matters)

p.s. What's the latest with modules/packages these days? Years ago, I started to implement a "package manager" on top of git, but the language wasn't really ready yet.

I have written wren-package and it's bundled by default with wren-console... and is used for all the Exercism tooling/exercises... it uses git and all dependencies are shallow, but it seems to work well for lots of things... but I think we're still in the early stages. Need more people to show up and want to use Wren on the console for real work I think.