lint-review icon indicating copy to clipboard operation
lint-review copied to clipboard

JSON linter not commenting on the correct line

Open adrianmoisey opened this issue 9 years ago • 6 comments

$ cat test.json
{
  "one": "value",
  "two": 2,
  "three": "three",
}
$ jsonlint test.json
test.json:5:0: Warning: Strict JSON does not allow a final comma in an object (dictionary) literal
   |  At line 5, column 0, offset 52
   |  Object started at line 1, column 0, offset 0 (AT-START)
test.json: ok, with warnings

If "three": "three" was added in the PR, lintreview wouldn't comment on this error, as it reports the error 1 line down.

I'm not sure what the solution so this problem is.

adrianmoisey avatar Apr 26 '16 06:04 adrianmoisey

I guess this is a JSON issue, and not a jsonlint issue, but maybe we should switch to the npm-installable one:

± cat test.json
{
  "one": "value",
  "two": 2,
  "three": "three",
}
± jsonlint -c test.json
test.json: line 4, col 19, found: '}' - expected: 'STRING'.

zoidyzoidzoid avatar Apr 26 '16 08:04 zoidyzoidzoid

I can't see public access to the source for demjson or a public issue tracker either.

zoidyzoidzoid avatar Apr 26 '16 08:04 zoidyzoidzoid

Seems legit:

$ jsonlint -c has_errors.json
has_errors.json: line 1, col 0, found: 'INVALID' - expected: 'STRING', 'NUMBER', 'NULL', 'TRUE', 'FALSE', '{', '['.
$ jsonlint -c has_warnings.json
has_warnings.json: line 2, col 8, found: 'INVALID' - expected: 'STRING', 'NUMBER', 'NULL', 'TRUE', 'FALSE', '{', '['.
$ jsonlint -c no_errors.json
{
  "one": "value",
  "two": 2
}

adrianmoisey avatar Apr 26 '16 08:04 adrianmoisey

I take that back.

The errors are definitely not as pretty, and it doesn't have have warnings. The npm one hasn't been updated in two years.

± jsonlint -c -s tests/fixtures/jsonlint/has_warnings.json
tests/fixtures/jsonlint/has_warnings.json: line 2, col 8, found: 'INVALID' - expected: 'STRING', 'NUMBER', 'NULL', 'TRUE', 'FALSE', '{', '['.

± virtualenv/bin/jsonlint tests/fixtures/jsonlint/has_warnings.json
tests/fixtures/jsonlint/has_warnings.json:2:9: Warning: String literals must use double quotation marks in strict JSON
   |  At line 2, column 9, offset 11
tests/fixtures/jsonlint/has_warnings.json:3:12: Warning: JSON does not allow identifiers to be used as strings: u'three'
   |  At line 3, column 12, offset 32
tests/fixtures/jsonlint/has_warnings.json:3:28: Warning: Strict JSON does not allow a final comma in an object (dictionary) literal
   |  At line 3, column 28, offset 48
   |  Object started at line 1, column 0, offset 0 (AT-START)
tests/fixtures/jsonlint/has_warnings.json: ok, with warnings

zoidyzoidzoid avatar Apr 26 '16 12:04 zoidyzoidzoid

Looking at how Yelp's pre-commit does checking if JSON is correct and compares it to pretty formatted JSON, maybe we should do this ourselves?

Though I guess as a standalone library?

zoidyzoidzoid avatar Apr 26 '16 12:04 zoidyzoidzoid

@zoidbergwill If you build a tool that can be integrated with lintreview, we could switch tools :smile:

markstory avatar Apr 26 '16 17:04 markstory