-
Notifications
You must be signed in to change notification settings - Fork 14
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
Octal config #29
Octal config #29
Conversation
- Linting: Add eslint with `recommended` and ignore file - Enforce 4 spaces indent
- Enforce consistent semicolons
* master: Remove package.lock - Fix: While in cr_escape mode, disable escaping after encountering newline # Conflicts: # lib/json6.js # package-lock.json
…ndent * commit '2333280b16d64ca1ae0f35b8066c8b25b2fd5511': Rework '\\\r\n' case. Remove comments Update git ignore to omit package-lock.json # Conflicts: # lib/json6.js
Sorry I'm slackin on this a bit; it's been a long time that I haven't had to change this. |
Re: purpose for debugging, ok, sure, gotcha. I've added a commit to the debug PR to revert the addition of a public 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 |
* master: Reset cr_escaped when stringEscape is set to false Fix indeation style Fix indentation brackets that were the first commentary about the code Fix syntax error of case in an extra layer of statement block Fix line-seprator and paragraph separator which should return to col=1. Fix \\ \r <any character> (Mac Line Endings) to reset col position; and fix skipped emit of character # Conflicts: # lib/json6.js
… logging (also improving test coverage)
- Testing: Bad character coverage (including one unexpectedly failing test for triple single quotes not throwing) - Testing: Octals (including one unexpectedly failing test for missing a character) - Testing: Object/arrays with various types (including one unexpectedly failing test for an array with an empty object) - Testing: Comments (including two unexpectedly failing for not being closed) - Testing: Special characters within keys - Testing: Non-matching keywords - Testing: Bump timeouts - Refactoring: Remove unused comment checking code (previous code block already handled presence of comment)
…lup will strip out
Squashed commits: [5e4ddf3] Fix indeation style [21d2c7f] Fix indentation brackets that were the first commentary about the code [08ac869] Fix syntax error of case in an extra layer of statement block [ea6a6fb] Fix line-seprator and paragraph separator which should return to col=1. Fix \\ \r <any character> (Mac Line Endings) to reset col position; and fix skipped emit of character
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. (there are spaces before the || continuation ... ) (for example)
|
…nfig * commit '6c6a6c146cde045a7b6d0000ab273af0eb561775': Reordered some lines; Fix trailing space; fix testing wrong unicode character values for LS,PS. # Conflicts: # lib/json6.js
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). |
…ish to convert these to tabs as well)
…es indent otherwise (in editorconfig)
I'm not so concerned with tabs or spaces in tests - certainly there should be a mix... (of even badly tabbed things) |
So like automated reformaters do this... https://github.com/d3x0r/JSON6/pull/29/files#diff-72f881adec77a0933be79785abb280efL578-L583 |
That link doesn't expand correctly - the json6.js diff is unloaded by default... |
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. |
Oh sorry, I broke octal; This all looks good to me. |
* master: Remove leading 0 octal interpretation Update README.md # Conflicts: # README.md # lib/json6.js
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 |
Also, there are still 9 tests not passing due to apparently all preexisting issues (including tests I recently added to highlight an apparent problem). |
failing tests = values '01234' should equal 1234 (not their octal value) a second place tests 0123 == 89
these are just broken... looking into them. I'll merge this and then work on those. |
yes if hex_char > 255 can never happen. (trigger to that state is [0-3] and 377 is the most it could be... (255)
There were a lot of toher debug lines left in there... I'm commenting those out too. |
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'
probably should re-add as a positive test somewhere (along with other keywords?) Updated document about comments that will now throw errors at end of document... Got all tests passing... keyword-fieldname positive test missing |
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. |
Re octal string escapes... |
at the cost of adding 4 cases to the switch, could just demote if(octal) to in the case of number characters?
hm. I do use \0. |
|
I think lone or unmatched surrogates are ok... JSON handles them: JSON.stringify('\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. |
Builds on #27, #28
While I could see this as a candidate for being set upon each
parse
call (unlike the proposeddebug
method which is even more likely to just need setting once):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 addednpm run test-cov
), are indicative of:istanbul ignore
statements can be added--preferably the more specificistanbul ignore if
/istanbul ignore else
for if/else statements rather thanistanbul ignore next
as they will only ignore one or the other (theif
or theelse
and not both))