Skip to content

Conversation

@florentchauveau
Copy link
Contributor

Hello,

When merging stderr with issues, the parts = line.split(":") line assumes that the path does not contain any : character. This breaks on Windows, and causes a SublimeLinter error:

SublimeLinter: ERROR: Unhandled exception:
Traceback (most recent call last):
  File "C:\Users\Florent\AppData\Roaming\Sublime Text 3\Installed Packages\SublimeLinter.sublime-package\lint/backend.py", line 157, in execute_lint_task
    errors = linter.lint(code, view_has_changed)
  File "C:\Users\Florent\AppData\Roaming\Sublime Text 3\Installed Packages\SublimeLinter.sublime-package\lint/linter.py", line 1021, in lint
    return self.filter_errors(self.parse_output(output, virtual_view))
  File "C:\Users\Florent\AppData\Roaming\Sublime Text 3\Installed Packages\SublimeLinter.sublime-package\lint/linter.py", line 1046, in filter_errors
    for error in errors
  File "C:\Users\Florent\AppData\Roaming\Sublime Text 3\Installed Packages\SublimeLinter.sublime-package\lint/linter.py", line 1045, in <listcomp>
    error
  File "C:\Users\Florent\AppData\Roaming\Sublime Text 3\Installed Packages\SublimeLinter.sublime-package\lint/linter.py", line 1084, in parse_output_via_regex
    for m in self.find_errors(output):
  File "C:\Users\Florent\AppData\Roaming\Sublime Text 3\Packages\SublimeLinter-golangcilint\linter.py", line 115, in find_errors
    issue = self._formalize(issue)
  File "C:\Users\Florent\AppData\Roaming\Sublime Text 3\Packages\SublimeLinter-golangcilint\linter.py", line 193, in _formalize
    issue["Pos"]["Line"] = int(issue["Pos"]["Line"])
ValueError: invalid literal for int() with base 10: ' C'

I have added a regex (with non greedy matching) that properly matches both Windows and Unix path.

If the regex does not match, the previous code is still ran.

Let me know if you have any comment.

@braver braver requested a review from cixtor July 19, 2019 11:48
@cixtor
Copy link
Member

cixtor commented Jul 20, 2019

Hello @florentchauveau thank you for your contribution.

I generally try to avoid using regular expressions are they usually decrease the performance of the algorithm, specially in this case where you are adding one inside an iteration, every warning in the report will significantly affect the linter.

However, since you already went ahead to write the code, rather than rewriting the whole thing to avoid another regular expression, I propose to invert the condition. If the split fails, then use the regular expression. This way your code will only run for Windows users, which according to your description of the issue are the only ones affected by this bug.

Let me know what do you think.

@florentchauveau
Copy link
Contributor Author

Hello @cixtor,

Sure! I just pushed a different implementation without a regex.

I also added a check of whether the string starts with typechecking error: or not. Definitely the case on my environment, but reading the code it appears it's not the case for everyone. Either it's with recent golangcilint versions, or Windows specific, I don't know.

Without the check, the filename would always be typechecking error (even with Unix path), because it's the first part of the split with :.

Let me know what you think.

Copy link
Member

@kaste kaste left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Hi. Python has rsplit see here https://docs.python.org/3/library/stdtypes.html#str.rsplit which takes a second arg to tell it how often you want to split max.

Other than that I think it is important to match the filename correctly. As it seems the plugin does not emit the filenames to SublimeLinter core which ideally it should. Not necessarily part of this PR. This is probably a bit tricky bc this plugin works with temporary directories. But if it is possible you should emit absolute paths and SublimeLinter will take that and show'em.

@florentchauveau
Copy link
Contributor Author

Hello @kaste, thank you for your review.

Using rsplit would not help I believe, because the error message may also contain :.

I am using split with a maximum split specified for both cases (unix and windows) which by the way was not specified before (and was probably returning partial messages!).

Not sure about the absolute path being returned or not, but typecheck errors are successfully shown by SublimeLinter on my environment. Not sure it should be part of this PR, I just wanted to fix the filename parsing :)

@kaste
Copy link
Member

kaste commented Jul 20, 2019

This plugin could report errors from multiple files in one go if it emits filenames along with the error. That's not for this PR but something @cixtor or a different PR could enable.

I only mention it here because you fix the filename in general. (I personally favor your regex approach as well because it more readable and handles all cases in a distinct way but I'm coming from SublimeLinter core and have nothing to say here expect tips and tricks.)

@cixtor cixtor merged commit bb3544f into SublimeLinter:master Jul 22, 2019
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Development

Successfully merging this pull request may close these issues.

3 participants