Skip to content
New issue

Have a question about this project? # for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “#”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? # to your account

replacer #339

Closed
sjpt opened this issue Apr 7, 2017 · 14 comments
Closed

replacer #339

sjpt opened this issue Apr 7, 2017 · 14 comments

Comments

@sjpt
Copy link

sjpt commented Apr 7, 2017

I needed a yaml dumper that included a replacer option similar to JSON.stringify.
This turned out to be a simple modification to js-yaml; there may be details not covered.
Its use changes the handling of undefined fields so that they are omitted from the yaml (similar to JSON).

If there is any interest in this change I will make it available.

I will probably add a reviver option for parsing.

@puzrin
Copy link
Member

puzrin commented Apr 7, 2017

I'm not sure this is needed here, but leave this open to collect feedback.

@sjpt
Copy link
Author

sjpt commented Apr 7, 2017

Thanks for the comment.

For background information: The reason we need the replacer is to prevent some objects getting placed in the output. We have an application using three.js, and many of my objects have associated THREE objects. However, when we dump the state we don't want to dump most of those, especially not the gory details of large vertex buffers, but rather recreate them after load.

We used to use JSON, but it was very noisy, especially for multiline strings with double quotes,;so awkward when we needed to edit by hand. (At least it wasn't nearly as bad as XML)

@puzrin
Copy link
Member

puzrin commented Apr 7, 2017

You could just create filtering function before calling yaml dump: yaml.safe_dump(your_filter(data))

@sjpt
Copy link
Author

sjpt commented Apr 7, 2017

I don't want to modify my data structure or clone the major part of it; which if I understand you right is what you expect your_filter(data) to do? Rather, just to omit certain objects from the dump.

@zakhenry
Copy link

In the same boat (wanting to discard undefined field); it is odd that the behaviour should differ from json as yaml is supposed to be a superset of json.

@graphicore
Copy link

See #391 for more discussion about this topic, includes also examples. Closed because it is a duplicate.

@graphicore
Copy link

graphicore commented Jan 23, 2018

Can we have this issue renamed to something like:

Feature Request: add optional replacer API like toJSON of JSON.stringify

@sjpt I'd suggest an option like {replacer: 'toJSON'}to turn on the feature and to configure it.

I'm still not sure if this feature is welcome.

If there is any interest in this change I will make it available.

If possible, could you please make it available as a PR (or as a branch in your fork), I'm interested in it anyways :-)

@sjpt
Copy link
Author

sjpt commented Jan 23, 2018

I'll try to make a PR soon. My replacer is like the global replacer of JSON, not like the toJSON object function. Both have their place. How about options {replacer: globalFunction, localReplacer: 'toJSON'}

Actually, I specifically want to avoid 'toJSON' discussed in #391 as it is misused in three.js so for example
JSON.stringify(new THREE.Texture())
JSON.stringify({ x: new THREE.Texture()})
throws an exception, as does serializing anything that indirectly contains a texture unless you explicitly avoid it with a replacer. (I raised an issue there years ago and never followed it up.)

@sjpt
Copy link
Author

sjpt commented Jan 23, 2018

Pull request created, I apologize if I have not done this correctly.

@graphicore
Copy link

Pull request created

PR is here: #392

I had a look and added some comments.

My replacer is like the global replacer of JSON

I see, you are talking about the replacer parameter of JSON.stringify. Awesome, I didn't know that :-)

JSON.stringify(value[, replacer[, space]])

I specifically want to avoid 'toJSON' discussed in #391 as it is misused in three.js

In this case, should I re-open #391 ?

Is that a problem of the 'toJSON' or of three.js. You can't avoid misuse of any API, I don't get how a bad usage of an API makes it undesirable. It's different if the API itself leads to bad design, that would be clearly a flaw. I'd like to see what's the case here, misuse of API or inherent bad API design/concept.

I'm looking for good arguments not to want//have/use/pursue toJSON, as I'm quite in favor of it. A good argument can change my mind.

I raised an issue there years ago and never followed it up.

Do you have a link to this issue. I'm interested in the discussion.

@graphicore
Copy link

graphicore commented Jan 24, 2018

I raised an issue there years ago and never followed it up.

Do you have a link to this issue. I'm interested in the discussion.

Found it: mrdoob/three.js#8428

With having an option to configure localReplacer this is a none-issue in here anyways. And, for three.js using a method toJSON with different semantics than JSON.stringify expects, this just means that something like JSON.stringify(new THREE.Texture()) is not a scenario intended by the three.js developers. Just don't do it.

@sjpt
Copy link
Author

sjpt commented Jan 24, 2018

Thanks for finding that three.js issue of mine, I was about to hunt for it.

I agree that configurable localReplacer makes this a non-issue now we are using yaml rather than JSON. In fact, for us the localReplacer is not needed at all, replacer does what we need.

Also, it is easy just not to do JSON.stringify(new THREE.Texture()) However, it is not easy to avoid an indirect equivalent, when the THREE.Texture is referenced deep down inside my own structures which I did want to JSON serialize (leaving out the references to THREE objects which we reconstruct after load).

I could make a deep copy of our structure without the THREE references and then serialize that, but that is much more work

@graphicore
Copy link

In fact, for us the localReplacer is not needed at all, replacer does what we need.

Not in my case, that is when the objects you serialize actually implement toJSON for use with JSON.stringify.

rlidwka added a commit that referenced this issue Dec 22, 2020
@rlidwka
Copy link
Member

rlidwka commented Dec 22, 2020

Added in 359b264 (currently in dev branch).

Tried to do this in a simplest possible way, so there're a few limitations:

  • no array syntax for replacer (it can be done via replacer function)
  • no toJSON (it can be done via replacer function)
  • no reviver (does someone need it?)

# for free to join this conversation on GitHub. Already have an account? # to comment
Labels
None yet
Projects
None yet
Development

No branches or pull requests

5 participants