Fix/one spawn per project #83

cgalvarez · 2018-02-25T19:34:56Z

This PR has the following goals:

Massive refactor of the code.
Analyze the whole project, not only the currently open file, so allowing for any engine and including previously missing issues like duplicated code.
Debounce the linting process, so multiple files from the same project can merge into one unique analysis.
Remove some code that doesn't make sense or it's outdated.
Fix all currently reported issues on CodeClimate. I've managed to comply with the cognitive complexity. Hope @Arcanemagus likes the final refactor 😉
Proper docblocks for each function.

And still waiting for the following bugs to be fixed in their respective repos:

There are some issues left that are very annoying, but I don't have any clue on how to fix them, and can be the subject of future PRs:

On the first Atom run (right after opening Atom for the first time or reloading it with control + shift + F5), the CLI is ran multiple times (typically two). The debounced linting cannot address this issue because the time between the executions exceeds 2s, and setting the debounce timeout to that feels overkilling, and make the whole point of this PR useless.
When starting Atom with multiple files open, changing from one to another still triggers a new analysis. This could be addressed to improve performance.
When starting Atom with multiple files open, the CodeClimate issues are generated correctly for AtomLinter, but they never get added. User need to manually re-trigger analysis by saving a file to get the issues included in AtomLinter pane.

Also, this PR...

Fixes #75
Fixes #76
Closes #77 with a temporary workaround
Fixes #81
Closes #69, since it should be supported by CodeClimate CLI in first place, and it should miss some issues (like duplicated code), which this PR aims to solve.
Closes #74
Closes #77
Closes #78

Edited by @Arcanemagus to automatically close issues/PRs.

cgalvarez · 2018-02-26T10:41:06Z

The tests seem to hang, cannot make them even print any error.

Anyone that could lend me a hand on this? This PR can be installed and tested locally with:

git clone -b 'fix/one-spawn-per-project' https://github.com/cgalvarez/linter-codeclimate.git linter-codeclimate-pr
cd ./linter-codeclimate-pr
apm link --dev
apm install
atom -d .

cgalvarez · 2018-02-26T15:19:33Z

Wow, I was about to give up, but the tests take ~273s on CI to finish!! Locally ~20s. What an amazing difference... but finally get it working and passing!! 🎉 🎉 🎉

EugeneGrigorenko · 2018-04-03T15:04:34Z

Is it gonna be merged?

Arcanemagus · 2018-04-06T04:44:57Z

Looks like the Code Climate people aren't maintaining this 😆😢.

Taking a look over this now, that diff summary looks daunting 😮.

Arcanemagus

Here's some issues, both logic and design, that I see from a quick first pass.

Arcanemagus · 2018-04-06T04:52:14Z

lib/index.js

@@ -1,88 +1,356 @@
 'use babel';

+/* eslint no-use-before-define:0 */


Is disabling this absolutely necessary? If so, this should be moved to the general ESLint configuration.

After making all functions that access to properties/methods of the package object, this can be removed.

Arcanemagus · 2018-04-06T05:02:05Z

lib/index.js

@@ -120,163 +384,14 @@ export default {
  },

  provideLinter() {
-    const Helpers = require('atom-linter');
-    const configurationFile = '.codeclimate.yml';
    return {
      name: 'Code Climate',
      grammarScopes: ['*'],
      scope: 'file',


The readme section saying this only worked for single file analysis has been removed... which implies that results for any file can be returned. If that's the case this should be changed to project.

Arcanemagus · 2018-04-06T05:07:49Z

lib/index.js

+  if (path.file === undefined) return path;
+
+  // Fetch previously cached paths when available.
+  if (ccLinter.openOnTextEditor[path.file]) {


This is accessing properties of the package object, just make this a method of that object.

Actually, ccLinter.openOnTextEditor is never populated, so this can be simplified to if (false), I'm guessing that's not what you intended?

You're right. I forgot to populate that prop to cache project root and textEditor.

Arcanemagus · 2018-04-06T05:12:30Z

lib/index.js

+    ignoreExitCode: true,
+  };
+
+  if (ccLinter.disableTimeout || global.waitsForPromise) {


This is accessing properties of the package object, just make this a method of that object.

global.waitsForPromise should be replaced with atom.inSpecMode().

Arcanemagus · 2018-04-06T05:13:22Z

lib/index.js

+const runCli = async (cwd) => {
+  const execOpts = {
+    cwd,
+    uniqueKey: cwd,


This should have the provider name added in case the user has multiple providers running on the same directory.

Totally agree. My fault!

I made the same mistake when first implementing uniqueKey usage, that's how I know it's an issue 😆.

Arcanemagus · 2018-04-06T05:57:25Z

lib/index.js

+  } catch (e) {
+    notifyError(e, 'Invalid JSON returned from CodeClimate. See the Console for details.');
+    // eslint-disable-next-line no-console
+    console.error('Invalid JSON returned from CodeClimate:', result);


Why not just console.error inside notifyError?

Arcanemagus · 2018-04-06T06:00:14Z

lib/index.js

+ */
+const fetchOpenFilepaths = () => {
+  const openFiles = {};
+  atom.workspace.textEditorRegistry.editors.forEach((textEditor) => {


atom.workspace.textEditorRegistry.editors is a private property, this should be atom.workspace.getTextEditors().

Also instead of calling this every single time, it might be better to do atom.workspace.observeTextEditors().

Arcanemagus · 2018-04-06T06:10:17Z

lib/index.js

+
+    // Exit early if issued file is not open
+    const file = join(path.project, issue.location.path);
+    if (!open[file]) return;


I would instead use something like findTextEditor to get the TextEditor for a file if one is available, if not return the best position you can. If the user then opens the file a TextEditor will be available and a more accurate position can be determined from that.

Once you start returning results for anything other than the active TextEditor that you are passed, you must change the scope to project, and since that allows you to send results for any path, there is no reason to only filter results to ones that the user has open.

Arcanemagus · 2018-04-06T06:12:55Z

lib/index.js

-    performance.clearMarks(startMark);
-  }
-  performance.mark(startMark);
+const configurationFile = '.codeclimate.yml';


This should just be declared directly where it is being used, if later on this becomes something that needs configuring it can be changed then.

Arcanemagus · 2018-04-06T06:14:02Z

lib/index.js

+const linting = {};
+const fingerprints = {};
+const debounceTimeout = 250;
+const mapSeverity = {


I would leave this down in the function where it is used.

cgalvarez · 2018-04-06T09:37:57Z

@Arcanemagus Thank you very much for detailed review. I'll take a look at all your reported issues ASAP and make a new commit to fix them all.

cgalvarez · 2018-04-16T17:10:05Z

I've just fixed all your reported issues. Let me know If I've missed something or you want me to make a rebase or something 😉

I think it would be desirable to disable some Code Climate engines run in CI to make tests run faster.

Arcanemagus

Great work, thanks a ton for taking this on!

Arcanemagus · 2018-04-21T05:32:59Z

lib/index.js

+
+    if (this.cache[path]) return;
+
+    textEditor.onDidDestroy(() => delete this.cache[path]);


Nice solution to keeping the cache but invalidating it when needed. The only potential issue is that you need to remember to re-populate it if the TextEditor's path is missing from the cache (which you did where needed 👍).

Arcanemagus · 2018-04-21T05:37:54Z

Since CI is passing, and I can't run this locally (Windows), I'll go ahead and merge this. 😆

Arcanemagus · 2018-04-21T06:13:24Z

@cgalvarez Assuming @AtomLinter/linter-codeclimate has no issues with it, if you want to be brought on as a maintainer for this project I would be fine with that 😉.

Arcanemagus · 2018-04-21T06:28:20Z

Published in v0.3.0 🎉.

cgalvarez · 2018-04-25T09:01:59Z

@Arcanemagus Great! I will be glad to help anytime I can.

cgalvarez mentioned this pull request Feb 26, 2018

Asyncify the specs #74

Closed

remove cli init, skip non-issues, massive refactor, asyncify tests

a81be15

cgalvarez mentioned this pull request Feb 26, 2018

remove cli init and skip non-issue reports #82

Closed

cgalvarez mentioned this pull request Mar 10, 2018

codeclimate linter does not work #85

Closed

Arcanemagus suggested changes Apr 6, 2018

View reviewed changes

Carlos García added 10 commits April 16, 2018 10:20

Move configurationFile to findProjectRoot

d237b5d

Use atom.inSpecMode() instead of global.waitsForPromise

90a2d0b

Make runCli() a method of ccLinter

db5d229

Change linter scope to project

1fbe397

Declare mapSeverity inside parseIssues()

0f6f3ff

Add provider name to unique exec key

3a34565

Declare execArgs inside ccLinter.runCli()

aca2557

Declare notificationDefaults inside notifyError()

fea0e9c

Make track() a method of ccLinter

fce512a

Move functions into linter object

7c028a4

Arcanemagus approved these changes Apr 21, 2018

View reviewed changes

Arcanemagus merged commit 1456860 into AtomLinter:master Apr 21, 2018

Arcanemagus mentioned this pull request Apr 21, 2018

Fix #76: discard non-issue messages #84

Closed

cgalvarez mentioned this pull request Apr 25, 2018

Initial Adoption of Continous Deployment using semantic-release #86

Merged

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Fix/one spawn per project #83

Fix/one spawn per project #83

cgalvarez commented Feb 25, 2018 •

edited by Arcanemagus

Loading

cgalvarez commented Feb 26, 2018

cgalvarez commented Feb 26, 2018

EugeneGrigorenko commented Apr 3, 2018

Arcanemagus commented Apr 6, 2018

Arcanemagus left a comment

Arcanemagus Apr 6, 2018

cgalvarez Apr 16, 2018

Arcanemagus Apr 6, 2018

Arcanemagus Apr 6, 2018

Arcanemagus Apr 6, 2018

cgalvarez Apr 16, 2018

Arcanemagus Apr 6, 2018

Arcanemagus Apr 6, 2018

Arcanemagus Apr 6, 2018

cgalvarez Apr 16, 2018

Arcanemagus Apr 21, 2018

Arcanemagus Apr 6, 2018

Arcanemagus Apr 6, 2018

Arcanemagus Apr 6, 2018

Arcanemagus Apr 6, 2018

Arcanemagus Apr 6, 2018

cgalvarez commented Apr 6, 2018

cgalvarez commented Apr 16, 2018

Arcanemagus left a comment

Arcanemagus Apr 21, 2018

Arcanemagus commented Apr 21, 2018

Arcanemagus commented Apr 21, 2018

Arcanemagus commented Apr 21, 2018

cgalvarez commented Apr 25, 2018

		@@ -1,88 +1,356 @@
		'use babel';

		/* eslint no-use-before-define:0 */


		if (this.cache[path]) return;

		textEditor.onDidDestroy(() => delete this.cache[path]);

Fix/one spawn per project #83

Fix/one spawn per project #83

Conversation

cgalvarez commented Feb 25, 2018 • edited by Arcanemagus Loading

cgalvarez commented Feb 26, 2018

cgalvarez commented Feb 26, 2018

EugeneGrigorenko commented Apr 3, 2018

Arcanemagus commented Apr 6, 2018

Arcanemagus left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

cgalvarez commented Apr 6, 2018

cgalvarez commented Apr 16, 2018

Arcanemagus left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Arcanemagus commented Apr 21, 2018

Arcanemagus commented Apr 21, 2018

Arcanemagus commented Apr 21, 2018

cgalvarez commented Apr 25, 2018

cgalvarez commented Feb 25, 2018 •

edited by Arcanemagus

Loading