Skip to content
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

Line numbers in stylish and json report are off by one #22

Open
rixx opened this issue Sep 7, 2020 · 7 comments
Open

Line numbers in stylish and json report are off by one #22

rixx opened this issue Sep 7, 2020 · 7 comments
Labels
bug Something isn't working
Milestone

Comments

@rixx
Copy link

rixx commented Sep 7, 2020

Describe the bug

All line numbers in the stylish and json report format are off by one, reporting line 0 when the actual issue is on line 1. In the compact export format, both versions of the line number are reported.

Steps to reproduce

Create a test.html with this content:

 <form></div>

For reference, I also tested this with indent errors by running against a file containing just <div></div>. The same issue occurs, but doesn't illustrate the issue with mixed content in some error messages.

Run curlylint test.html. The output reports line number 0:

test.html
0:8	Parse error: expected 'form' at 0:8	parse_error

Oh no! 💥 💔 💥
1 error reported

Run curlylint --format json test.html. The output reports line number 0:

[{"file_path": "test.html", "line": 0, "column": 8, "message": "Parse error: expected 'form' at 0:8", "code": "parse_error"}]
Oh no! 💥 💔 💥
1 error reported

Run curlylint --format compact. The output reports line number 1 and line number 0:

test.html:1:8: Parse error: expected 'form' at 0:8 (parse_error)
Oh no! 💥 💔 💥
1 error reported

Expected behavior

Consistent reporting of line numbers, preferably 1-indexed. 😉

Actual behavior

Behaviour is buggy/mixed, primarily 0-indexed, as described above. Sorry, no screenshots, but I pasted the output.

Reproducible demo

As above. Your issue template indicates that I should link a project for this, which I can do, if the one-line test.html is insufficient. Please let me know, in that case.

Bug source

The compact method casts its contents to str (1), which comes down to the IssueLocation class, which includes a +1 in its str() method. The other two formatters don't adjust the internal line number at all.

The issue of the mixed message in the compact report seems to sit deeper, as it's due to the issue message, which is constructed in each rule.

@rixx rixx added the bug Something isn't working label Sep 7, 2020
@thibaudcolas
Copy link
Owner

Hey @rixx, thank you for the detailed report. No need for the reproducible demo in this case – this is plenty enough of information (and something I’ve noticed myself as well, occasionally).

I vaguely remember fixing with the half-assed + 1 as you describe, but need to check whether there might be a more appropriate place for this to be fixed, so it’s not up to individual formatters to access the location information there right way. Or be happy with it and just make sure all the built-in formatters do this consistently.

@mikaraunio
Copy link

For me, json and stylish give correct, 1-indexed line numbers, whereas the + 1 in IssueLocation causes compact to have 2-indexed line numbers.

Have submitted PR #47 to remove the + 1.

@rixx
Copy link
Author

rixx commented Jan 7, 2021

I cannot reproduce this. I just installed curlylint from PyPI (0.12), and I still get the following:

> curlylint test.html
test.html
0:9	Parse error: expected 'form' at 0:9	parse_error

> curlylint test.html --format json
[{"file_path": "test.html", "line": 0, "column": 9, "message": "Parse error: expected 'form' at 0:9", "code": "parse_error"}]

@mikaraunio
Copy link

This is strange, I get the following (also with a regular file, not just stdin), on MacOS with Python 3.9 as well as Debian 10 with Python 3.7.3. This is with my patched fork, stock curlylint shows -:2:0: with --compact.

$ echo "<test" | curlylint --format json -
[{"file_path": "-", "line": 1, "column": 0, "message": "Parse error: expected one of '>', 'attribute', '{#', '{%', '{{' at 1:0", "code": "parse_error"}]
$ echo "<test" | curlylint --format compact -
-:1:0: Parse error: expected one of '>', 'attribute', '{#', '{%', '{{' at 1:0 (parse_error)

@thibaudcolas
Copy link
Owner

Well, the mystery thickens! I thought last time I had investigated this that compact was correct and the rest was 0-indexed as initially reported here. I’ll have another look. I think I had it almost solved but got blocked by not being able to decide where it was better to convert the numbers to be 1-indexed – at the reports level, or directly where the issues are created.

@mikaraunio
Copy link

Thicken it does! I tried the original sample input from @rixx, and it seems that the heart of the issue is actually that line numbers from some errors are 0-indexed, while others are 1-indexed:

$ echo "<form></div>" | curlylint -q --format json -
[{"file_path": "-", "line": 0, "column": 8, "message": "Parse error: expected 'form' at 0:8", "code": "parse_error"}]

$ echo "<test" | curlylint -q --format json -
[{"file_path": "-", "line": 1, "column": 0, "message": "Parse error: expected one of '>', 'attribute', '{#', '{%', '{{' at 1:0", "code": "parse_error"}]

$ echo "{% if %}{% endfor %}" | curlylint -q --format json -
[{"file_path": "-", "line": 0, "column": 11, "message": "Parse error: expected one of 'autoescape', 'block', 'blocktrans', 'comment', 'elif', 'else', 'endif', 'filter', 'for', 'if', 'ifchanged', 'ifequal', 'ifnotequal', 'not an intermediate Jinja tag name', 'spaceless', 'verbatim', 'with' at 0:11", "code": "parse_error"}]

$ echo "{% if %}" | curlylint -q --format json -
[{"file_path": "-", "line": 1, "column": 0, "message": "Parse error: expected one of '<!--', '<![^>]*>', '<', '[^{<]+', 'any character', '{#', '{%', '{{' at 1:0", "code": "parse_error"}]

@mikaraunio
Copy link

Ah, looks like this mystery is in fact just coincidence. @rixx's sample used mismatched tags, whereas my sample (and the error that initially led me to comment) was an unterminated tag.

Mismatched tags will report the line of the closing tag, but unterminated ones will of course make the scan move forward - in the case of my examples above, to the empty line introduced by echo. This makes it obvious:

echo -e 'Line0<form></div>\nLine1\nLine2' | curlylint -q --format json -
[{"file_path": "-", "line": 0, "column": 13, "message": "Parse error: expected 'form' at 0:13", "code": "parse_error"}]

echo -e 'Line0<form><div>\nLine1\nLine2' | curlylint -q --format json -
[{"file_path": "-", "line": 3, "column": 0, "message": "Parse error: expected one of '<!--', '<![^>]*>', '<', '</', '[^{<]+', 'any character', '{#', '{%', '{{' at 3:0", "code": "parse_error"}]

Therefore now fully agree with @rixx's original report, and second his suggestion that line numbers in json and stylish output, and in the message body, be brought in line with compact output's 1-indexed numbers.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
bug Something isn't working
Projects
None yet
Development

No branches or pull requests

3 participants