Octal config

[brettz9]

Builds on #27, #28

While I could see this as a candidate for being set upon each parse call (unlike the proposed debug method which is even more likely to just need setting once):

1. I don't think it is that critical (nor hard to disable/reenable after a call) if calling code needs to support octals in some cases and forbid it in others.
2. It would mess up the familiar JSON.parse style as it would need added config.

Btw, though I could take a still deeper look, I think after this PR, the code could really benefit from your taking a look at whether the remaining statements in the code that are still not covered (looks like 28 statements only now--which, if you're not familiar with nyc, you can see in /coverage/lcov-report after running the script I added npm run test-cov), are indicative of:

1. Bugs that need fixing (I think at least one or two of the failing tests are due to these actually)
2. Code that can be removed
3. Guards that are not necessary now but you just want to keep for sanity checking (and for which istanbul ignore statements can be added--preferably the more specific istanbul ignore if /istanbul ignore else for if/else statements rather than istanbul ignore next as they will only ignore one or the other (the if or the else and not both))

[d3x0r]

Sorry I'm slackin on this a bit; it's been a long time that I haven't had to change this. The purpose of the consts to disable debug, is then it's not even compiled and is an optimization issue... making it flexible; although making tooling happier; has micro-deoptimizations...

[brettz9]

Re: purpose for debugging, ok, sure, gotcha.

I've added a commit to the debug PR to revert the addition of a public debug method and add instead an internal log method which can be ignored both by Rollup and nyc (while avoiding adding the need for istanbul ignore statements everywhere the logging is called)--if you think that approach will work for you.

By the way, re: the eslint changes, if you didn't want to have to review all of those minor changes of inserted semi-colons and converted tabs manually--yet were ok trusting eslint--you could just add the eslintrc config (or I could add a PR that only adds that), and then you can yourself run eslint --fix ..

[d3x0r]

I prefer strict tab indentation. Tab up to the appropriate column/level, and space fill to align continued content. Tabs never follow a space. Tabs are never after the start of the line. (the first file; style is set indent_style=space). I see, however, that most expressions are already on one line and continuations don't apply

(there are spaces before the || continuation ... ) (for example)

                        if( cInt == 32/*' '*/ || cInt == 160/* &nbsp */ || cInt == 13 || cInt == 10 || cInt == 9
                          || cInt == 0xFEFF || cInt == 44/*','*/ || cInt == 125/*'}'*/ || cInt == 93/*']'*/
                          || cInt == 58/*':'*/ ) {
                            break;

[brettz9]

Ok, due to the complexity of having to apply to the eslint PR and then add these changes on top of that, I've instead added a commit to this PR to use initial tabs (eslint calls your favored approach of allowing for spaces following tabs, "smart-tabs", which I've allowed for through eslint).

[d3x0r]

I'm not so concerned with tabs or spaces in tests - certainly there should be a mix... (of even badly tabbed things)

[d3x0r]

So like automated reformaters do this...

https://github.com/d3x0r/JSON6/pull/29/files#diff-72f881adec77a0933be79785abb280efL578-L583 which will tab expand badly based on user tab choice.

[d3x0r]

That link doesn't expand correctly - the json6.js diff is unloaded by default... lines L578-L583

[brettz9]

As far as tests, I can understand not wanting them for fixtures and what not, but the tests themselves are otherwise still a part of the project. Did you see any instances where I had accidentally modified tabs/spaces within fixtures or test strings?

I've added a commit to fix a few cases such as the one you highlighted.

[d3x0r]

Oh sorry, I broke octal; This all looks good to me. I replaced 'const' with 'let' and the result was the same today(on node 12.13.1 x64). I'll merge this; though one last request, can you just like comment out log() lines? And any //log() lines can be double commented? '////log' ? I don't see a lot of use in these anymore except in comments... and they can be find-replaced to reenabled?

[brettz9]

Ok, I've merged, commented out log lines (and logging code to avoid getting unused errors), and added tests for line/paragraph separators and carriage return within a key, thereby improving coverage further.

So if it's ok with you, I think it can be merged.

However, on running npm run test-cov, coverage is still not at 100% (97% of statements), so if you (or I) get a chance, it'd be great to either ignore any further unreachable guard code, or to add tests for these. I'd personally be more interested in the missing coverage for lines 365, 371-378, 398, and 798 (and for coverage of the else paths of 428, 805, 870). (The rest seem more to do with status, stack or queue, though these would also be good to cover.)

[brettz9]

Also, there are still 9 tests not passing due to apparently all preexisting issues (including tests I recently added to highlight an apparent problem).

[d3x0r]

failing tests = values '01234' should equal 1234 (not their octal value) a second place tests 0123 == 89

o = parse( "{ true:1 }" ); I'm going to revise the documentation, and allow unquoted keywords as object keys; they will become their string equivalent.

these are just broken... looking into them. o = parse( "{ a : 'no quote' [1] }" );
parser.write( "true false null undefined NaN Infinity" ); is dropping 'false, null, undefined' looks like I have some if( val ) test...

I'll merge this and then work on those.

[d3x0r]

yes if hex_char > 255 can never happen. (trigger to that state is [0-3] and 377 is the most it could be... (255) Put the other error checks in the right place...

        o = parse( "'\\x1'" );
        o = parse( "'\\01'" );
        o = parse( "'\\u31'" );
        o = parse( "'\\u{0'" );

There were a lot of toher debug lines left in there... I'm commenting those out too.

[d3x0r]

/* should be an error? I would think I would rather document that end of document is also a valid close to a comment...'//' (missing \n) or '/*' .. and looks like, by logic '/' at the end of the document is a valid comment?

Sort of have to leave it that way... because of the streaming nature; it could get the rest of the comment in another message...

I had to remove this in 'bad tests'

describe('Bad tests', function () {

    /*
    it('Unquoted keyword', function () {
        expect(function () {
            o = parse( "{ true:1 }" );
            console.log( "got back:", o );
        }).to.throw(Error);
    });
    */

probably should re-add as a positive test somewhere (along with other keywords?) I updated the readme to indicate unquoted keywords will be accepted as strings for object field names...

Updated document about comments that will now throw errors at end of document...

Got all tests passing...

---

keyword-fieldname positive test missing (err... there's something about octal string escapes that's missing too I think; I frget

[brettz9]

Re: tests passing, great!! Nice too re: README updates...

Good re: removing bad bad test--just added to help focus how you actually wanted behavior to be in the project.

I have a minor PR plan to submit shortly.

Re: Octal string escapes--It seems incongruous to me to remove octal escapes yet keep them in strings... Both are forbidden in strict mode.

[d3x0r]

Re octal string escapes... I concur somewhat, with (in the spirit of following ECMA). 1) stringify doesn't emit such nonsense. 2) it doesn't collide with anything else (ie decimal... I've never seen \13 \10 ? I debated myself a bit at the time.
I don't tend to use them.

[d3x0r]

at the cost of adding 4 cases to the switch, could just demote if(octal) to in the case of number characters? what do I do with \uDC00 - \UDFFF ?
Maybe it's OK

a = '\ud800';
"�"
a += '\udc00'
"𐀀"

hm. I do use \0.

[d3x0r]

"use strict"; "\0ad"
" ad"

"use strict"; "\01ad"
VM140:1 Uncaught SyntaxError: Octal escape sequences are not allowed in strict mode.

[brettz9]

I think lone or unmatched surrogates are ok... JSON handles them:

JSON.stringify('\udc00')

"\"\udc00\""

No difference in strict mode.

But yeah, no octals there. With ES6 Modules going into implied strict mode, I think it especially makes sense to follow that.