Cleanup

[d3x0r]

@brettz9 re #29 and #28 commits to 29 are also in 28 (or vice versa so I'm closing 28.

I would also like to fixup tests to make sure they are meaningful.

Opinion

{ true : 'false' } // fail or not?

On the one hand, it's in a place where only a string can be (really shouldn't allow numbers, except as strings to conform behavior-ly to JSON... The code just does a 'recoverIdent' which based on the accumluator of letters might just have been 'tru' which is accumulated as a state into the constant 'true' value (rather than having comparisons for that)... so the 'recoverIdent' just returns the string constant for basically any of the constants its collected. I can update the documentation.

should I try to create an object with true as a key? I don't know that's even possible.

There is JSON6.parser() (and JSON6.stringifier)

Can you perhaps look at ... https://github.com/d3x0r/jsox https://github.com/d3x0r/JSOX/blob/master/lib/jsox.js#L1831-L2239 This is the stringifier there. It can be stripped to remove ... Hmm... prototype classes and I guess 'partialClass' .

https://github.com/d3x0r/jsox#usage (same sort of API as JSON6)

[d3x0r]

Re LS and PS (Line separator and paragraph separator...)

                        case 13/*'\r'*/:
                            cr_escaped = true;
                            pos.col = 1;
                            continue;
                        case 10/*'\n'*/:
                            if( cr_escaped ) {
                                // \\ \r \n
                                cr_escaped = false;
                            } else {
                                // \\ \n
                                pos.col = 1; // no return to get newline reset, so reset line pos.
                            }
                            // fall through, increment line
                        case 2028: // LS (Line separator)
                        case 2029: // PS (paragraph separate)
                            pos.line++;
                            break;

should really be

                        case 13/*'\r'*/:
                            cr_escaped = true;
                            pos.col = 1;
                            continue;
                        case 10/*'\n'*/:
                            if( cr_escaped ) {
                                // \\ \r \n
                                cr_escaped = false;
                            } else {
                                // \\ \n
                            // fall through, increment line
                        case 2028: // LS (Line separator)
                        case 2029: // PS (paragraph separate)
                                pos.col = 1; // no return to get newline reset, so reset line pos.
                            }
                            pos.line++;
                            break;

yes ?

[d3x0r]

https://github.com/d3x0r/JSON6/commit/ea6a6fbddaeb4b85af148f686c75dd34f0d15eec

[d3x0r]

that particular commit was wrong syntax and caused build failures; subsequently fixed. Reformats applied to { opening on an empty line. Fixed cr_escaped not clearing at end of string. I do like what you've done with building the unicode identity table; although I have really sort of demoted that to an advisory; treating the JSON6/JSOX encoding as more ASCII control characters framing arbitrary bytes that don't otherwise match control characters. Whitespace \x20 , \xa0 (nbsp) being of course control characters... it simplifies the encoding; otherwise a sparse lookup has to be done per character and that's very expensive....

[brettz9]

Re: closing #28, sure. However, #27 is still open and #29 builds on that too. Do you think you could look to merge at least #27 first (if not ready for merging #29) before making other changes to master, as it is getting a bit troublesome to do diffs and merges given how much the eslint PR affects (albeit trivial stuff).

As far as the additional linting changes, I can enforce the brace-style in eslint if you are ok with it, but it seems there are currently two styles you have in use per https://eslint.org/docs/rules/brace-style , both "1tbs" (no line break after closing brace of if succeeded by else) and "stroustrup" (line break after closing brace of if succeeded by else). Would you prefer one or the other? (the most common in JS seems to be "1tbs".) And for the allowSingleLine option, do you want to forbid that (since some of your recent changes seem to have gone away from that, and I think it may help with readability), or do you want to explicitly allow it (since there are a number of cases where this still occurs)?

Re: { true : 'false' } // fail or not?...

I very much think a big part of JSON6' special utility is accepting (data-based) JavaScript as is, so those familiar with JS can just use it. Since this passes in JS (tested in the browser console), and is in object-like construct, I'm good with it building an object (and would prefer it to be so, if possible).

Btw, it would be very helpful to have a summary of the ways in which JSON6 is, per the README, a superset of JavaScript (allowing what JS does not), as opposed to the aspects in which it fits as a strict subset of JS.

(Also, since "superset" is mentioned on the README, it might be also helpful for the README to clarify and use the language explicitly that although JSON6 is not a strict subset of JavaScript, it is a "strict" superset of JSON, and apparently JSON5.)

As far as the other issues, I'm mostly just trying to honor what you have in place already. I haven't examined or considered too deeply what is going on beyond noting that some blocks are not being reached. Speaking of which, I've tacked on your CR changes on master into my PR (manually since rebasing was not giving easy to view diffs). However, it seems that coverage is still being missed (I haven't looked recently at what the changes should be). Have you tried running npm run test-cov (after npm install) and looking at the resulting coverage/lcov-report/ in the browser? You can re-run the tests after making changes to see whether the tests are getting the lines covered. But again, it'd be nice if we could look at fixing the remaining coverage problems like this after the eslint (and ideally octal) PR could be considered/merged?

Thanks!

[brettz9]

I've merged recent changes into #27 which, as mentioned there, I hope you can review before making any changes. There still seem to be coverage issues with the carriage return code that was added (though I haven't tried to see at this point whether they can be covered or not), but if you review #27 and #30 here, it will be a lot easier to check yourself whether changes fix the remaining coverage issues or not.

[brettz9]

Btw, I don't know what the behavior you want here to be, but these tests get full coverage on the separator and carriage return code you recently changed:

it('Parses encapsulated key with escaped carriage return', function () {
    var result = JSON6.parse( "{ 'my  \\\r  key':3}" );

    console.log( "result:", result );
    expect(result).to.deep.equal({
        'my    key': 3
    });
});

describe('Separators', function () {
    it('Line separator', function () {
        var result = JSON6.parse( '"\\\u2028"' );
        expect(result).to.equal('');
    });
    it('Paragraph separator', function () {
        var result = JSON6.parse( '"\\\u2029"' );
        expect(result).to.equal('');
    });
});

[d3x0r]

Oh 0x2028, not 2028. The behavior fixed such a bahavior of cr_escaped always dropped output, although cr_escaped should not have been set if there was a valid cr character after the escape.

[d3x0r]

that's a two part problem... the other one needed to fix the other one.. Isn't the first one right?

[d3x0r]

I know there's a lot of changes...

https://github.com/d3x0r/JSON6/pull/29/files#diff-abfec2dea1fce5373e60bc597e2a5911

these benchmarks that have timeouts - I'd probably refactor afterward to do each loop based on a max-time run instead of absolute sample count - samples/time is the same either way...