Fix #6441. #6442
Fix #6441. #6442
Conversation
Add check for the line number to be positive.
|
@busykai Or would it make more sense to just require linting extensions to provide a valid line number? We're still not guarding against line numbers are are past the end of the file... I'd be more inclined to not silently treat all those cases as ok... |
|
@peterflynn I guess it might be a reasonable requirement (to provide a valid line number), but I think besides just documenting it, it might worth enforcing it as well. Do you want me to add check for a valid line range too? |
|
@peterflynn I forgot to mention: if there's an issue with the provider itself which it is capable of reporting, I believe it still should appear in the problem panel (just because there's no other place right now). This way a casual user will be able to address it without going to the console (e.g. JSHint may report "Bad option" if there's an erroneous option or "Failed to parse config" if there's a syntax error in the config file). My intent was to address this situation rather than enforce any requirement to the provider. The check for NaN that you added serves the same purpose, I believe, I just went a little bit further. |
|
@peterflynn here is an easy way to check for valid line number. To check the numeric value, lineCount needs to be accessible which could be easily avoided by just asking document to get the line from the editor. JSLint does not complain, but if assignment in the conditionals are not welcome, can change it... |
ghost
assigned 
|
To @peterflynn - seems worth landing in this sprint since it looks small and if I'm reading the bug correctly it affects existing extensions. |
|
@peterflynn any chance of landing this before Sprint 36 closes? |
|
@peterflynn I'll take this |
|
@busykai I think it might make sense to display nothing (empty string) instead of the '0', if the line number is invalid. What do you think? Line number '0' looks odd. |
|
@ingorichter done! yes, it did look odd. |
|
@busykai Thanks. Merged! |
| // some inspectors don't always provide a line number or report a negative line number | ||
| if (!isNaN(error.pos.line) && | ||
| (error.pos.line + 1) > 0 && | ||
| (error.codeSnippet = currentDoc.getLine(error.pos.line)) !== undefined) { |
There was a problem hiding this comment.
@busykai @ingorichter I had a concern that this silently hides bugs in extensions. I'd prefer it if we don't accept NaN and -1 as line numbers. Can we file a bug on whatever extension has this problem, and then remove this code once that extension is fixed?
There was a problem hiding this comment.
@peterflynn it's understandable, so what do you think the strategy should be for linters to report their internal issues? just log it to console? see this issue for example: cfjedimaster/brackets-jshint#24 with this change, the way the output looks likes is the following:
which is much nicer than seeing "nothing to lint".
Now, I understand your point of view. I do think, however, that having the checks for valid input in the code does not actually contradict to the requirement to the extension to report things better. For example, this message "bad option" could have been reported as meta instead of error (on the other hand, having only meta would not cause problem panel to pop up).
So with this background what your suggestion would be for a long-term proper solution? Note that I also filed #6460 to as an idea to address this.
Add check for the line number to be positive.