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

ValueError with numeric keys #58

Closed
mborsetti opened this issue Aug 8, 2022 · 6 comments
Closed

ValueError with numeric keys #58

mborsetti opened this issue Aug 8, 2022 · 6 comments

Comments

@mborsetti
Copy link

JavaScript variables in the "wild" often have keys in numeric form. As an example, here's an extract of one from a webpage at www.sec.gov:

{"10-12B":"a|3|Registration of securities [Section 12(b)]",144:"a|2|Report of proposed sale of securities"}

Unfortunately this seems to choke the library:

>>> import json5
>>> json5.loads('{"10-12B":"a|3|Registration of securities [Section 12(b)]",144:"a|2|Report of proposed sale of securities"}')
ValueError: <string>:1 Unexpected "4" at column 61

Not sure if this is a library design choice (not mentioned in the docs) or an oversight, but thankfully in the interim the demjson3 library does work with them.

@mborsetti mborsetti changed the title Numeric keys ValueError with numeric keys Aug 8, 2022
@dpranke
Copy link
Owner

dpranke commented Aug 8, 2022

Thanks for mentioning this issue!

Integer keys are legal in regular JavaScript, but they are not permitted in the json5 specification, that this library implements.

At the moment, the docs kinda assume that you're familiar with JSON5, but given how the docs are written now, I can certainly see how you might think that more stuff from JS was legal than is actually the case.

I'd be happy to update the docs somehow to make the prohibition clearer. If you have suggestions for how to do so, I'm happy to hear them. Or, I can take a shot at it, and I'd be fine with mentioning the demjson3 library as an alternative.

I would be reluctant to change the code to allow integer keys without buy-in from the folks that own the JSON5 spec, because that might encourage people to create more illegal json5 docs. Alternatively, I could add an optional flag to allow them, but I wouldn't even really want to do that unless there was clear demand for this from many users.

Any thoughts on the above?

Side note: can you provide a full URL to wherever you're finding that snippet of code? I didn't actually see that string in the source of https://www.sec.gov, so I wasn't sure if you were saying it was literally on that page, or just somewhere on the SEC site.

@mborsetti
Copy link
Author

Hello @dpranke,

Thanks for a comprehensive response! I indeed looked for a specification for JSON5 to understand where the issue was, but it points to ECMAScript 5.1 and IdentifierName, which I never heard of, and stopped the search there to avoid entering a rabbit hole.

My motive was to consolidate dependencies after finding json5 from being loaded by another dependency. I saw in the docs the reference to JavaScript, so I replaced demjson3 (which does not seem to support JSON5, at least from the docs) with it, and it worked fine and forgot about it, until by chance it had to parse the submissionForms variable at https://www.sec.gov/edgar/browse/scripts/lookup-data.min.js and got this Exception.

It's your project so I can't really advise as to how you want to proceed, but having a universal tool to read (not necessarily write) JSON, JSON5 and "in the wild" JavaScript JSON would be super useful.; if you want to maintain purity, one way is indeed to add an optional flag, or, even better, issue a warning and add a flag to suppress it. If not, then I would definitely advise editing the docs to be explicit that it does not support "in the wild" JavaScript JSON.

Thanks!

@dpranke
Copy link
Owner

dpranke commented Aug 18, 2022

Whoops, sorry for going radio silent on you :(.

I think at the moment I'm inclined to not add anything to support this. This library is definitely not intended to be used to parse regular JavaScript, and that's what the stuff in that link is.

If others can come up with good use cases for this, I'm happy to revisit the issue.

Does that make sense? Sorry for not wanting to support what you're doing.

@mborsetti
Copy link
Author

No problem and thanks. Your project, your rules, am cool with it! I do would like to suggest that you remove/rephrase the reference to JavaScript on the homepage so that others don't make the wrong conclusion like I did!

@dpranke
Copy link
Owner

dpranke commented Aug 18, 2022

That's a good suggestion about the docs, I'll add that. Thanks!

dpranke added a commit that referenced this issue Aug 18, 2022
This library only supports JSON5 documents. It cannot
parse everything that is legal JavaScript. For example,
an object with an integer key is legal in JS, but
not in JSON5. I've updated the text to reflect this.
@dpranke
Copy link
Owner

dpranke commented Aug 18, 2022

Done in e28f7b9 / 0.9.10.

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

No branches or pull requests

2 participants