d3x0r / JSON6

JSON for Humans (ES6)
Other
236 stars 14 forks source link

Linting tests #31

Closed brettz9 closed 4 years ago

brettz9 commented 4 years ago

~Btw, there seems to be one regression, though not reflected in tests. For some reason, when the package.json file is built, it adds a section about rollup-plugin-terser which I don't see any reason it should be built. But I'd appreciate if you could look at this short PR first as I think the testing procedures here ought to be helpful in preventing some regressions going forward (at least if CI is heeded; one could also add a commit hook to lint there if you like).~ Filed test for this in #33 which, if fixed, should fix this underlying package.json issue as well.

brettz9 commented 4 years ago

I just added a couple unquoted keyword tests.

brettz9 commented 4 years ago

also updated to add:

brettz9 commented 4 years ago

I've also just added:


brettz9 commented 4 years ago

Btw, were you saying that for line 452, if( hex_char_len < 2 || ( stringUnicode && hex_char_len < 4 ) ) {, the else path here couldn't be taken? (and I could therefore remove or comment out that check)?

d3x0r commented 4 years ago

Btw, were you saying that for line 452, if( hex_char_len < 2 || ( stringUnicode && hex_char_len < 4 ) ) {, the else path here couldn't be taken? (and I could therefore remove or comment out that check)?

I was not saying that... there was an octal check < 255 that was superfluous.

                                hex_char_len++;
                                if( stringUnicode ) {
                                    if( hex_char_len == 4 ) {
                                        val.string += String.fromCodePoint( hex_char );
                                        stringUnicode = false;
                                        stringEscape = false;
                                    }
                                }
                                else if( hex_char_len == 2 ) {
                                    val.string += String.fromCodePoint( hex_char );
                                    stringHex = false;
                                    stringEscape = false;
                                }

the else if will happen, when the second hex (\x) character is found...

d3x0r commented 4 years ago
        it('should recover with carriage return escape at end of string', function () {
            var o = JSON6.parse( '"\\\r"' );
            console.log( "o is", o, typeof o );
---         expect(o).to.equal('\r');
+++         expect(o).to.equal('');
        });

the fixed cr_escape handling recognizes it was consumed.... The sorts of things that failed then were like \\rA (because cr_escaped was still set and the A got eaten... it should be "A" now

brettz9 commented 4 years ago

Thanks for the merge!

As far as the else... it is actually not visited despite having a two-character hex.

I think this is because, within that if( hex_char_len < 2 || ( stringUnicode && hex_char_len < 4 ) ) { block, there is this:

                                else if( hex_char_len == 2 ) {
                                    val.string += String.fromCodePoint( hex_char );
                                    stringHex = false;
                                    stringEscape = false;
                                }

By setting stringHex to false, when revisiting the block which contains both of these blocks, else if( stringHex || stringUnicode ) { will no longer be true, so the else path of the middle check will not be reachable.

d3x0r commented 4 years ago

That above was really because hex_char_len < 2 || ( stringUnicode && hex_char_len < 4 ) is always true...

                            if( stringUnicode ) {
                            }else if( hex_char_len == 2 ) {

doesn't complain.