-
-
Notifications
You must be signed in to change notification settings - Fork 6
Wrapper to strip pseudo-JSON comments from an io::Read input stream #24
Comments
Although I appreciate the proposed separation of concerns, you basically have to parse the JSON file twice, right? For example, to implement If |
I don't think this is the case. The only relevant piece of state for the wrapper is whether it is inside of a string literal or not. There doesn't need to be any parsing of I think the amount of parsing dedicated to |
I guess that's true if the JSON is well-formed. If it isn't, I suspect yielding a proper parse error becomes trickier. But also: what about the next logical feature, which is trailing commas? I consider it strongly tied to comments because if you don't support trailing commas and you have this:
and you comment a single line:
For JSON files that may be edited by hand where comments are supported, it's pretty common to see people prefer trailing commas (even though they're optional) to make this sort of thing easier. |
I would leave the error messages to serde_json rather than diagnosing syntax errors in the wrapper. Implementation note: we should sub out the comments with spaces, rather than removing them entirely, so that serde_json's error reporting of the line and column position will accurately apply to the input data. For trailing commas, it still only needs the "am I in a string" amount of state. If not inside of a string, then |
Sort of. For example: I would say that |
Ah, good call. If it's simpler to fork serde_json and integrate these changes into the existing parsing code, I would go ahead with that. |
@dtolnay That sounds fair, thanks! I discovered that there is also https://crates.io/crates/json5, but it depends on https://crates.io/crates/pest so it can use a PEG. I was trying to avoid additional dependencies, but perhaps I should give that a shot. Also, https://crates.io/crates/json5 offers |
I put together a simple crate for this: https://crates.io/crates/json_comments |
Thanks @tmccombs, this is terrific! I filed several issues to follow up on the API and documentation. |
Oh, perhaps I should have mentioned that I did end up making that fork: https://github.com/bolinfest/serde-jsonrc As explained in the README, it supports |
Occasionally people ask for JSON deserialization with comments, without wanting to go all the way to JSON5 or Hjson.
This doesn't belong in serde_json which is intended strictly for JSON as described by https://json.org/.
I would like to be able to write:
Roughly the API would be:
The text was updated successfully, but these errors were encountered: