SublimeLinter 4 and improvements by meengit · Pull Request #3 · qltysh-archive/SublimeLinter-contrib-codeclimate

meengit · 2020-06-23T12:15:38Z

Hi!

This Pull Request is a follow up of the Pull Request #2 of @schuylr. It brings various fixes and improvements:

Support for SublimeLinter 4 (current)
SublimeText Project is no longer required, the linter will detect its context also on open folders and files
Updated Readme.md including examples

I hope to hear from you to bring this package forward. Please let me know if you are okay with my changes or if you expect any adjustments.

Thank you very much!

braver · 2020-12-23T12:24:29Z

Just a little heads-up. This package seems to be abandoned. We intend to replace its entry in Sublime Text package control with a fork that will be made compatible with SublimeLinter 4.

kaste · 2020-12-23T19:46:52Z

+        'selector': (
+            'source.css, '
+            'source.go, '
+            'source.haskall, '


-> source.haskell

kaste · 2020-12-23T19:49:39Z

-    executable = "codeclimate"
+    defaults = {
+        'chdir': "${project}",
+        'executable': 'codeclimate',


Probably no need to set this, it's automatically chosen by the SL framework as it's the first item in the list you return from cmd().

kaste · 2020-12-23T19:51:08Z

-    )
-    executable = "codeclimate"
+    defaults = {
+        'chdir': "${project}",


Confusing to have a chdir as we already have working_dir. What's the difference between them, when do you want to set different things here, one for working_dir, one for chdir. We also don't change a dir.

kaste · 2020-12-23T19:52:28Z

-    comment_re = None
+
+    root = None
+    path = None


You actually never access self.root or self.path so initializing here is probably not necessary.

kaste · 2020-12-23T20:15:09Z

+
+        root = re.search(r"\{(\w+)\}", self.defaults['chdir']).group(1)
+        path = self.filename.replace(self.context.get(root) + '/', '')
+        return ['codeclimate', 'analyze', path]


So generally, you must or want to set the relative path to the current file. Relative to what? Usually the working_dir, no? (I don't see this requirement in the codeclimate docs btw.)

You can just get the working dir using self.get_working_dir() and compute the relative path to the filename, for example using os.path.relpath(self.filename, self.get_working_dir()) (add import os at the top of the file) as it is Windows compatible. (Although it looks like codeclimate is not either compatible or as easy to use on Windows.) As long as it's a file-only-linter this could work as you don't have to expect None values for filename or cwd. You do have to expect that the user opens random files within one window though. So a check that filename is actually under control of the folder seems reasonable.

Generally return ["codeclimate", "analyze", "$args", path] t.i. somehow telling where user args should be injected is usually a good idea.

Generally, you probably don't want to read in the defaults (self.defaults[...]). You do want to read the actual, final settings using self.settings[...]. Otherwise you don't get the values the user specified but always the defaults. Setting (mutating) self.defaults is probably not what you want for the same reasons.

Thank you very much for your review @kaste.

Your explanations helped me a lot to better understand how SublimeLinter works. I tried to improve my work based on your comments. Please feel free to request further changes.

Review fixes

kaste · 2020-12-24T11:58:42Z

+        else:
+            command = 'codeclimate'
+
+        return [command, 'analyze', '${args}', '${file}']


Now it's so easy you don't have to define a method at all. executable is handled by the SL framework.

Just define cmd = ('codeclimate', 'analyze', '${args}', '${file_on_disk}'), maybe right before defaults for readability. file_on_disk holds the same value as file but also acts as a marker for SublimeLinter.

Hi @kaste

Thank you very much for your feedback.

Yes, that looks very easy. However, I noticed an additional issue. In some cases, I had results where shown in the wrong file – something like "the linter is showing File A's results in File B."
In short, I believe this effect appears because the file path is absolute and not relative to the working directory. (If you want to drive deeper into this, I believe qltysh/qlty#845 gives an overview of some of the key points.)

However, I solved the issue in doing this (short version without exception handling):

path = os.path.relpath(self.filename, self.get_working_dir()) return ['codeclimate', 'analyze', '${args}', path]

However, SublimeLinter appends the absolute path automatically. So, what I expect is:

INFO:SublimeLinter.lint.linter:Running ... /Users/run/Desktop/TEST (working dir) $ /usr/local/bin/codeclimate analyze src/Plugin/Field/FieldWidget/ACLConfigSelector.php

But what I get at the moment is:

INFO:SublimeLinter.lint.linter:Running ... /Users/run/Desktop/TEST (working dir) $ /usr/local/bin/codeclimate analyze src/Plugin/Field/FieldWidget/ACLConfigSelector.php /Users/run/Desktop/TEST/src/Plugin/Field/FieldWidget/ACLConfigSelector.php

I tried to get off this "automatic" automatic appending in several ways, but I found no way to solve it. What do I wrong?

Ah, the automatic filename appending... That's a shitty part of SublimeLinter we couldn't get rid of for compatibility reasons. We probably can change that in 2021.

We need a marker in the cmd, iirc return ["codeclimate", ..., "${xoo}"] should work. (T.i. any unused, undefined marker.)

Hi @kaste

Thank you very much for your answer. I'm not sure if I understand you correctly; or if my Python skills are not strong enough, to handle this "marker" thing.

I tried

def cmd(self): file = os.path.relpath(self.filename, self.get_working_dir()) return ['codeclimate', 'analyze', '${args}', file, '${xoo}']

and, surprisingly, this works. But it looks to me horribly wrong. May you have a working example for me?

Yeah, it's kinda horrible but easy to use. When we see a marker like $file_on_disk or anything else we do not append the filename automatically. When you use $xoo it will resolve to the empty string so this hack works afair. (Note that this plugin is the only plugin using this hack, all other plugins either use file_path or file_on_disk as all of them work with absolute file paths.)

The auto-appending code is actually in the finalize_cmd method but there is no need to implement/override this method as it's more complicated and brittle I think.

Hi @kaste

Oh, ok… I'm relieved.

I updated the command and appended the marker '${xoo}'. I also tried to catch someone trying to run the linter against a file from outside the working directory*. For this purpose, I check with if not (abs_path.startswith(os.path.abspath(working_dir) + os.sep)) if the current file is in the working directory. If it is not, I try to print a logger.info message and preventing SublimeLinter from linting the current file (return None). As far as I can see, it works but raises a backend error. Can I solve this case more accurately?

I plan to eliminate the limitation in further releases, and I see several solutions and a bunch of work. For now, I only try to get this plugin to work together with SublimeLinter 4.

* I'm doing this because of the "the linter is showing File A's results in File B"-issue when codeclimate is called with an absolute file path or from outside the working directory.

Review fixes

kaste · 2020-12-27T22:16:19Z

+
+        if not (abs_path.startswith(os.path.abspath(working_dir) + os.sep)):
+            logger.info(msg)
+            return None


Instead of return None which is not valid, raise PermanentError("file not part of the working dir") (error message up to you). At the top add from SublimeLinter.lint import PermanentError.

You probably want to call self.notify_unassign() before the raise to not show any errors to the user, basically to unassign the linter from the view. (Just as logger.info does only print in debug mode.) If you want to indicate an error to the user use self.notify_failure() instead and logger.warn. (I don't think that's a use case here because the user cannot fix this, we just ignore arbitrary views from outside the main folder.)

I wonder if it's enough to set

defaults = { ... "excludes": ["!${folder::}*"] }

t.i. exclude any filenames not starting with ${folder}. And since folder can be empty, with a fallback ${folder::} since no path can start with a :.

Ah, maybe not that good since a user can change this.

I added the exclusion statement, and it works as predicted. If I have opened a directory and then open a single file from another directory, the linter will not run:

SublimeLinter: linter.py:1042 codeclimate skipped '/Users/run/Desktop/ExternalFormatter.php', excluded by '!/Users/run/Desktop/TEST*' INFO:SublimeLinter.lint.linter:codeclimate skipped '/Users/run/Desktop/ExternalFormatter.php', excluded by '!/Users/run/Desktop/TEST*'

I also updated my exception handling to:

msg = 'The file \'%s\' is not part of the working dir (%s). ' \ 'Please see the Linter\'s README.md to get further ' \ 'instructions.' % (abs_path, working_dir) if not (abs_path.startswith(os.path.abspath(working_dir) + os.sep)): self.notify_unassign() raise PermanentError(msg)

I know it is a bit redundant along with the exclusion statement, but I suggest keeping it in place just in case…

Ah, you need to log the message using the logger as we don't log the error message, sorry that was probably misleading. (In PermanentError(msg) the msg is not for the user; you could not set a msg as well.)

I don't think the excludes trick is worth it; as the user can set the working_dir manually, in that case our excludes ($folder) would not work or still point to a different directory.

I updated the exception handling in doing a logger.warning first and then and then unassign and raise the permanent error in 281e165.

Ah yes, you're right. I removed the exclusion in c44c78a.

kaste · 2020-12-27T22:16:58Z


-from SublimeLinter.lint import Linter, util, persist
+
+logger = logging.getLogger('SublimeLinter.plugin.eslint')


eslint -> codeclimate

eslint is a different linter.

I removed the logger messages for now.

kaste · 2020-12-27T22:20:10Z

+            'text.html'
+        )
+    }
    regex = r'^(?P<line>\d+)(?:-\d+)?:\s(?P<message>.+)$'


If possible try to extract the filename the error is coming from to the group (?P<filename>). If that's possible SublimeLinter will handle errors from multiple files without any warnings.

The output of codeclimate is:

== src/Plugin/Field/FieldType/ACLFieldsItem.php (1 issue) == 29: Define a constant instead of duplicating this literal "value" 3 times. [sonar-php]

I set multiline to true and updated the regexp to:

regex = r'^== ((?P<filename>.*)(?= $\d+ issue$ ==))( $\d+ issue$ ==\n(?P<line>\d+))(?:-\d+)?:\s(?P<message>.+)$'

As far as I can see, it is working as expected. The doubled "(\d+ issue)" isn't that nice, but I found no other way to exclude the text from the file path with a positive lookahead and then set the second start in the same position for the rest.

The file name here looks like a header; I doubt codeclimate will duplicate the filename information; so this won't extract multiple issues from one file I think. Have you tested this?

Oh… Oh! Yes, it is. Thank you for your close inspection. I reverted the regex in b7fa651.

@kaste, I suggest preparing my repository to file the PR to overtake this linter plugin's maintenance in the next days. I will let the master branch, pointing to this pull request, as is, and switch with the protected branch to a newly created branch called main for further development.

kaste · 2020-12-27T22:28:41Z

+
+Suppose you use a `.codeclimate.yml`-configuration file. In that case, the `codeclimate` CLI needs to be executed in your configuration file's directory. Otherwise, it can't detect your configuration and runs only the default analyzes.
+
+SublimeLinter takes the (first) open folder (or the folder of a single opened file) as the working directory for executing its linter commands. You can change this behavior by setting the working directory of execution:


To clarify: It is possible to have multiple folders "open" in Sublime in one project/window. SublimeLinter will then take the first matching folder, t.i. usually a folder the file belongs to.

@kaste, thank you very much for your awesome support. I also updated the docs. Do you see further improvements in my PR?

Review fixes

kaste

LGTM! Ping @braver for how we actually take/transition the repo. Afair we only need a new release tag on the master branch of your repo.

kaste · 2020-12-30T23:14:42Z

Beside the code review I have a question. This linter adapter registers for a lot of languages, are these supported out of the box actually, or downloaded and updated on the fly? For example they download some code on first request. (Iirc haskell is a GB on my machine.)

The docs state that piping in the source code is supported. Why is this here a saved-file only linter?

braver · 2021-01-01T16:23:16Z


 ## Installation
-SublimeLinter 3 must be installed in order to use this plugin. If SublimeLinter 3 is not installed, please follow the instructions [here][installation].
+SublimeLinter 4 must be installed in order to use this plugin. If SublimeLinter 4 is not installed, please follow the instructions [here][installation].


Or simply "SublimeLinter" without the "4". If you look for SublimeLinter in package control it's not super obvious it's v4 or v3. Nobody has v3 installed "by accident".

braver · 2021-01-01T16:28:40Z

how we actually take/transition the repo

We need to change the package control entry to point to your fork (so create a PR there!) and then in your fork you need to create a release that's > 1.0.0 (so 2.0.0 or 1.1.0, your pick).

meengit · 2021-01-03T00:22:09Z

Hi @kaste

As far as I understand, codeclimate does not install engines automatically. From the documentation:

As part of setting up your project, we recommend running codeclimate engines:install from within your repository before running codeclimate analyze, and after adding any new plugins to your configuration file.

I did a set of simple performance tests and think it is not that good to run the codeclimate CLI on each random file that will be opened in SublimeText.

I suggest removing all selectors from the defaults and letting the user decide which selectors they want to use and do these settings globally or in a SublimeText Project File.

From my point of view, we're now talking about changes that go beyond this pull request. It is no longer just about compatibility with SublimeLinter 4, but about further interventions. Therefore, I have decided to leave this pull request as it is and make the changes in a new branch in my repository. I've added you as a maintainer to my repository and as a reviewer to the corresponding pull request, which you can find here.

meengit · 2021-01-03T00:23:01Z

Hi @braver, thank you very much for your update. I will file my Pull Request once @kaste has proven the changes here.

meengit · 2021-01-04T15:32:02Z

Hi @braver, my fork is now ready to replace the original package. I filed my pull request to overtake the responsibility.

@codeclimate, I'm open to giving you back the control over this package in reverting the package control index. If you are not interested, I would appreciate a deprecation notice or something similar at your repository's README.md.

Thank you very much for developing the codeclimate CLI. It is an excellent tool and helps me every day to improve my code.

Schuyler Jager and others added 11 commits January 6, 2020 14:22

Fix deprecations

cb59992

Clean up the rest of deprecations and errors

bbb9bed

Only run structure/duplication

14f180a

Fix codeclimate command to use engines correctly

3ff70ae

Fix js scope for js/jsx files

38cc8d9

Updated cmd

33d2c30

Set folder or file_path as working dir

a17fd42

Updated docs

41fcee0

Fixed file path, must be relative to working dir

42c2145

Added example for global args

4cb246c

Updated settings and examples

7314e2d

meengit mentioned this pull request Dec 20, 2020

SublimeLinter-contrib-codeclimate SublimeLinter/SublimeLinter#1778

Closed

kaste suggested changes Dec 23, 2020

View reviewed changes

meengit added 5 commits December 23, 2020 21:49

Fixed typo in source.haskell

84d5732

Removed initialization of root and path

fabc1f8

Cleaned up of linter command and working directory

7961905

Updated docs

354aea6

Merge pull request #1 from meengit/review-fixes

e44f271

Review fixes

kaste reviewed Dec 24, 2020

View reviewed changes

meengit added 3 commits December 27, 2020 22:27

Updated command & error handling

b000bc8

Cleaned up command handling

b781a48

Merge pull request #2 from meengit/review-fixes

0c7d527

Review fixes

kaste suggested changes Dec 27, 2020

View reviewed changes

kaste reviewed Dec 27, 2020

View reviewed changes

meengit added 5 commits December 28, 2020 00:03

Updated exception handling

159dd24

Updated linter regex (now multiline)

e19c458

Added exclusion if file is not in working dir

987473f

Updated docs

8ec7aa6

Merge pull request #3 from meengit/review-fixes

29e9483

Review fixes

meengit added 4 commits December 29, 2020 21:16

Removed/Reverted folder exclusion

c44c78a

Reverted regex pattern

b7fa651

Updated logging and error messages

281e165

Merge pull request #4 from meengit/review-fixes

05597a8

Review fixes

kaste approved these changes Dec 30, 2020

View reviewed changes

braver reviewed Jan 1, 2021

View reviewed changes

meengit mentioned this pull request Jan 4, 2021

Updated codeclimate SublimeLinter/package_control_channel#102

Merged


		from SublimeLinter.lint import Linter, util, persist

		logger = logging.getLogger('SublimeLinter.plugin.eslint')


		Suppose you use a `.codeclimate.yml`-configuration file. In that case, the `codeclimate` CLI needs to be executed in your configuration file's directory. Otherwise, it can't detect your configuration and runs only the default analyzes.

		SublimeLinter takes the (first) open folder (or the folder of a single opened file) as the working directory for executing its linter commands. You can change this behavior by setting the working directory of execution:

Conversation

meengit commented Jun 23, 2020 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

braver commented Dec 23, 2020

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

meengit Dec 26, 2020 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

kaste left a comment

Choose a reason for hiding this comment

Uh oh!

kaste commented Dec 30, 2020

Uh oh!

braver Jan 1, 2021 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

braver commented Jan 1, 2021

Uh oh!

meengit commented Jan 3, 2021

Uh oh!

meengit commented Jan 3, 2021

Uh oh!

meengit commented Jan 4, 2021

Uh oh!

Reviewers

meengit commented Jun 23, 2020 •

edited

Loading

meengit Dec 26, 2020 •

edited

Loading

braver Jan 1, 2021 •

edited

Loading