Fix duplicated issues and upgrade to linter v2 #65

cgalvarez · 2017-04-19T16:21:43Z

I'm using this package to lint PHP files. The result from command line for a given file is:

Starting analysis
Running phpmd: Done!

== functions.php (7 issues) ==
20-36: The variable $parent_style is not named in camelCase. [phpmd]
20-36: The variable $parent_style is not named in camelCase. [phpmd]
20-36: The variable $parent_style is not named in camelCase. [phpmd]
33-35: The method cge_cgesite_theme_enqueue_styles uses an else expression. Else is never necessary and you can simplify the code to work without else. [phpmd]
37-47: The variable $parent_style is not named in camelCase. [phpmd]
37-47: The variable $parent_style is not named in camelCase. [phpmd]
37-47: The variable $parent_style is not named in camelCase. [phpmd]

Analysis complete! Found 7 issues.

As you can see, there are repeated issues. The problem is that when clicking on the linter badges to show the errors/warnings, I get the following warnings in the console, due to the repeated issues returned by CodeClimate.

Besides, there are really less issues that shown in the badges. I see no benefit from getting the linter queue populated with repeated errors. That's one of the goals of this PR: removing repeated errors.

Also, I would like to distinguish the severity of the returned issues instead tagging them as Warnings by default. The severity of the issues is a feature introduced past month, and I think it would be a desirable and beneficial feature of this linter.

Finally, I think being able to read the CodeClimate engine that reported the issue would be beneficial too. That's why it appears at the beginning of each enqueued issue, with the check inside brackets. I've been digging into the CodeClimate sources and have seen only two severities so far: major (which maps to Error) and minor (which maps to Warning). Linter v2 only sets the severity, so I've dropped the type message option.

I've used the CodeClimate data types spec for Issues and the linter v2 spec for messages.

So, resuming, this PR does the following:

Removes duplicated issues based on their fingerprints, so the errors/warnings from the console are gone and the linter badges counts are correct now.
Tags issues by severity, not type.
Shows the CodeClimate engine that reports the issue along with the check name.
Inserts a link to perform a google search on the reported issue if user needs thorough info.
Provide a long description of the issue reported as appears in the web reports.
Switches linter-codeclimate to linter v2.

Here's a screenshot that shows the final appearance:

cgalvarez · 2017-04-19T16:42:42Z

A test is failing because it uses the v1 specs...

Arcanemagus · 2017-04-19T17:11:19Z

lib/index.js

+            severity: mapSeverity[issue.severity] || 'warning',
+            excerpt: `${issue.engine_name.toUpperCase()} [${issue.check_name}]: ${issue.description}`,
+            description: (issue.content && issue.content.body) ? issue.content.body : undefined,
+            url: `${issue.engine_name} ${issue.check_name}`,


This needs to be a real URL, as in https://domain.tld/path/to/page.

(The search functionality I'm guessing you were taking advantage of here was removed in v1.2.3 of linter-ui-default)

Arcanemagus · 2017-04-19T17:12:37Z

lib/index.js

-            filePath,
-            range,
+            severity: mapSeverity[issue.severity] || 'warning',
+            excerpt: `${issue.engine_name.toUpperCase()} [${issue.check_name}]: ${issue.description}`,


Does this match codeclimate's default output? Not necessarily requesting a change here, just wondering if all the extra information will be too much for some people and needs to be put behind a flag.

In the last screenshot:

issue.engine_name.toUpperCase() = PHPMD.

issue.check_name = Controversial/CamelCaseVariableName.

issue.description = The variable $parentStyle is not named in camelCase.

It allows me to know the engine that is throwing the issue (PHPMD), the ruleset and the rule that are throwing the issue (from the check_name) and an explanation of the issue (description).

It is true that after appending the whole CodeClimate detailed explanation in Markdown format, maybe it is not so crucial, because you can see it unfolding the extra information (the message.description is an accordion, it's not shown by default).

In the end I suppose it's just a matter of preference. Maybe another member of this package could provide us his/her POV 😇

just wondering if ... needs to be put behind a flag.

What do you mean? How would you do that?

So personally I would add all that info as well, however a lot of people complain when "extra" stuff is added like that, which is why I was asking if that matched how codeclimate itself output the information.

What do you mean? How would you do that?

In a few other providers things like the rule ("check") name are put behind optional flags, allowing people to hide them if they don't feel the need for them. So in implementing that you would just do an if on the state of that flag option.

Hold off on implementing all that though since I'd probably be fine with all the information there, but you might want to put the check_name at the end of the line since the message (tied with the location) is probably sufficient to fix the problem.

Arcanemagus · 2017-04-19T17:13:09Z

A test is failing because it uses the v1 specs...

Yep, fixing the specs is part of any v2 update 😉.

cgalvarez · 2017-04-19T17:59:43Z

@Arcanemagus I'm trying to test the package locally with atom --test spec, but I always get an annoying timeout: timed out after 5000 msec waiting for promise to be resolved or rejected.

I've been reading this thread, where you have taken part, but still can't fix it. Any idea to fix the timeout issue?

Arcanemagus · 2017-04-19T18:13:45Z

This package isn't utilizing that functionality (currently), so that's not the issue here. If you push what you currently have up I can take a look.

cgalvarez · 2017-04-19T18:31:45Z

I've sent all my changes. I know the timeout it's not the issue that makes Travis fail, but I wanted to test the specs locally before sending my changes, that's all. I'm pretty newbie to the world of Atom packages, and wanted to learn how to debug packages the right way 😉 and avoid bothering someone else with untested PRs.

Arcanemagus · 2017-04-19T22:39:45Z

Hmmm, I'm a bit confused now as the specs are passing in CI, and I don't see anything wrong with them locally. Is what you pushed up still giving you problems?

Btw, I'm almost always on Atom's Slack if that would be helpful to diagnose stuff like this 😉.

Arcanemagus · 2017-04-19T22:42:36Z

lib/index.js

+            severity: mapSeverity[issue.severity] || 'warning',
+            excerpt: `${issue.engine_name.toUpperCase()}: ${issue.description} [${issue.check_name}]`,
+            description: (issue.content && issue.content.body) ? issue.content.body : undefined,
+            url: encodeURI(`https:www.google.com/#q=${issue.engine_name} ${issue.check_name}`),


I'd rather not generate a search term like this at all. If the linter doesn't give you a URL, or you can't generate an exact URL from the rule name (like is done here for ESLint rules), then it shouldn't be included at all.

The issues with generating a search term is that then every single message "has" a URL, and in this case it isn't providing much benefit as the use can just copy/paste from the message.

I will remove it then. Generating a customized URL for each reported issue for each engine would be a time-consuming task right now, and CodeClimate seems to do it for us in the detailed explanation of the issue 😉

Arcanemagus · 2017-04-19T22:44:06Z

lib/index.js

@@ -267,11 +274,21 @@ export default {
            range = Helpers.generateRange(textEditor, line);
          }

+          // avoid duplicated issues


"avoid" -> "Avoid"

cgalvarez · 2017-04-20T09:34:15Z

The specs failed due to a timeout issue before failing locally due to the old v1 syntax. I updated the specs, but could not be able to test them because of that issue, that is described in the same Atom's forum thread that I linked previously in this comment.

I don't know the reason of the timeout, but I'm glad that the specs succeeded! 👏

All the requested changes have been commited.

Arcanemagus · 2017-04-20T16:02:14Z

the same Atom's forum thread that I linked previously in this comment.

That thread has to do with activationHooks, which has nothing to do with this package. The error message is the generic "Promise didn't resolve before the timeout" message, which is confusing since it works just fine here on CI.

Is this working locally for you or not currently?

cgalvarez · 2017-04-20T16:30:11Z

The PR works for me locally, but testing is NOT working for me locally. This is what I do to test the package:

$ atom --test spec
[5375:0420/182751:ERROR:browser_main_loop.cc(231)] Running without the SUID sandbox! See https://chromium.googlesource.com/chromium/src/+/master/docs/linux_suid_sandbox_development.md for more information on developing with the sandbox on.
[5375:0420/182757:INFO:CONSOLE(234)] "linter-codeclimate:: Command: `codeclimate analyze -f json -e fixme -e rubocop cool_code.rb </dev/null`", source: /opt/cgalvarez/linter-codeclimate/lib/index.js (234)
F

The codeclimate provider for Linter
  it works with a valid .codeclimate.yml file
    timeout: timed out after 5000 msec waiting for promise to be resolved or rejected


Finished in 5.491 seconds
1 test, 1 assertion, 1 failure, 0 skipped

cgalvarez · 2017-05-04T11:15:33Z

@Arcanemagus Is there anything pending to merge this PR?

Arcanemagus · 2017-05-04T16:26:42Z

You figuring out why the specs are failing locally for you but passing elsewhere? I hadn't done a re-review yet as I was waiting on that.

cgalvarez · 2017-05-05T09:26:52Z

Sorry, I've been absent some days and completely forgot that.

Well, after reading Writing Specs @ Atom Flight Manual, I've checked that the timeout has to do with my device. All I have to do is increasing the timeout of the waitsForPromise() invocation, modifying it from waitsForPromise(() => to waitsForPromise({ timeout: 10000 }, () =>, and then the tests succeed.

I've seen that @pointlessone achieved the same solution on PR #67. The tests always fail if I don't explicitly set that option, because as said in the previous docs link: the default value of the timeout options is calc as: process.env.CI ? 60000 : 5000, so in Travis it was being tested with a timeout of 60 sec, while for locally was being tests with a timeout of 5 sec. That's why it was always failing locally and succeeding in Travis, and that's why it is failing for @pointlessone too on PR #67.

I think it has to do with my own environment, but, as I am not the only one facing this usse, we could change it if you agree it could avoid future issues.

pointlessone · 2017-05-05T12:38:38Z

@cgalvarez The duplicates appear to be CC engine issue. Could you please report it there?

Arcanemagus · 2017-05-05T16:32:34Z

default value of timeout is process.env.CI ? 60000 : 5000

Now that is just plain bizarre, I've always had specs that get locked up fail after 10 seconds, not 5. The docs definitely match the code though, so I'm not sure how I always see 10 seconds 😮.

Arcanemagus · 2017-05-05T16:39:31Z

The duplicates appear to be CC engine issue.

That's an excellent point, if codeclimate itself is showing these duplicates it should probably be fixed there. I'd say the duplicate code should either be removed, or a note put in that it needs to be removed as soon as that is fixed on their end.

cgalvarez · 2017-05-05T17:29:25Z

@pointlessone I only get the duplicated issues for phpmd engine, so it is possibly an engine related issue, not a codeclimate one. Are you getting duplicated issues for a different engine?

cgalvarez · 2017-05-05T17:49:08Z

I'm not sure I always see 10 seconds 😮

Maybe you refer to the 10 seconds timeout for spawning a process with Helpers 😄 , which is used by default by all the AtomLinter ecosystem, but it is referred to spawning a process (typically executing the linter command), not executing tests.

Arcanemagus · 2017-05-05T22:42:30Z

Maybe you refer to the 10 seconds timeout for spawning a process with Helpers

Nope, definitely the one for specs in Atom, just hit this here working on linter-jshint 😛:

pointlessone · 2017-05-06T08:11:33Z

waitsForPromise uses jasmin's waitsFor. And that has timeout of 5 seconds. I can't explain why it's 10 seconds for you.

Arcanemagus · 2017-05-07T00:32:31Z

Me either 😛 Just saying why I was saying it was 10 seconds 😉.

cgalvarez · 2017-05-09T11:46:44Z

I've detected why the errors are appearing as duplicated for phpmd. phpmd itself is reporting three different issues as the same, because it is encapsulating it between the lines of its scope.

I'm using this example file:

<?php
function cge_theme_enqueue_styles() {
	$parent_style = 'twentyfifteen-style';
	wp_enqueue_style( $parent_style, get_template_directory_uri() . '/style.css' );
	wp_enqueue_style(
		'cge-site-style',
		get_stylesheet_directory_uri() . '/style.css',
		array( $parent_style ),
		wp_get_theme()->get( 'Version' )
	);
}

The results returned by PHPMD are:

<?xml version="1.0" encoding="UTF-8" ?>
<pmd version="@project.version@" timestamp="2017-05-09T11:36:35+00:00">
  <file name="/opt/cgalvarez/wp-theme-cge-site/test.php">
    <violation beginline="2" endline="11" rule="CamelCaseVariableName" ruleset="Controversial Rules" package="+global" externalInfoUrl="#" function="cge_theme_enqueue_styles" priority="1">
      The variable $parent_style is not named in camelCase.
    </violation>
    <violation beginline="2" endline="11" rule="CamelCaseVariableName" ruleset="Controversial Rules" package="+global" externalInfoUrl="#" function="cge_theme_enqueue_styles" priority="1">
      The variable $parent_style is not named in camelCase.
    </violation>
    <violation beginline="2" endline="11" rule="CamelCaseVariableName" ruleset="Controversial Rules" package="+global" externalInfoUrl="#" function="cge_theme_enqueue_styles" priority="1">
      The variable $parent_style is not named in camelCase.
    </violation>
  </file>
</pmd>

The variable $parent_style appears 3 times, so phpmd reports the issue 3 times, but all of them are provided with the same begin line 2 and end line 11, which is the function scope in which the variable is used, thus CodeClimate treats it like the same issue and assigns the same fingerprint.

I've open an issue at phpmd/phpmd/issues/467, but since the PHPMD team has 117 open issues right now, I think we could leave this fingerprint check until fixed, with a TODO comment as a reminder as you proposed previously @Arcanemagus

cgalvarez · 2017-05-09T12:09:24Z

Added the reminder TODO and included a special minor fix that was making the duration of the codeclimate command to not show in the logs. You can take any Travis log from another merged PR and check it.

cgalvarez · 2017-05-09T14:45:31Z

Well, now this PR fails too due to the same codeclimate error as #67...

cgalvarez · 2017-05-16T11:15:17Z

@Arcanemagus I've tested this PR with the latest Code Climate 0.63.5 and it works now. There are no errors shown neither inside the Atom IDE nor when passing the tests through the command line with atom --test spec.

How can I re-run the CI tests? Am I forced to make another commit?

pointlessone · 2017-05-16T11:18:48Z

You can do that. Or change some of the commits in this PR and force push. Or ask repo owner to rerun the build (they have a button for that).

cgalvarez · 2017-05-16T13:11:47Z

Thanks @pointlessone. I was searching that button, but didn't see it, and you've told me why 😉

Just discovered that I can make an empty commit and rebase the last commit, so I do not need to change any previous commit, with:

git commit --amend --allow-empty
git push --force

Arcanemagus · 2017-05-31T19:06:03Z

Since this brings in a requirement of Linter v2, I'd like to bring in automatically installing that version in case there are users that both ignore updates (many of them) and don't already have another provider doing this for them.

You can accomplish this by:

Bringing in the atom-package-deps module (npm install --save atom-package-deps)
Specifying a minimum of linter v2 in package.json like this
Calling the version check in an idle callback (so it doesn't increase the "activation" time of this package) like this. Make sure you:
- Dispose of any remaining idle callbacks in deactivate like this.
- Bump the minimum version of Atom to at least v1.7.0 here

cgalvarez · 2017-06-09T09:20:19Z

@Arcanemagus Requested changes implemented. It seems that changes in package.json requires someone with write access to resolve them manually. Please, let me know if there is anything more I can help with.

Arcanemagus · 2017-06-12T17:18:26Z

@cgalvarez I just fixed the merge conflict, but for future reference you could have fixed that yourself by either merging master into this branch and pushing the changes up, or rebasing this branch on top of the current master. 😉

Arcanemagus · 2017-06-12T17:21:20Z

Ha, and proving the point of why a blind "resolution" isn't the best, I introduced some dependencies back that were removed in master 😛.

Arcanemagus · 2017-06-12T17:28:35Z

Thanks for sticking through with this @cgalvarez!

cgalvarez · 2017-06-13T08:30:02Z

@Arcanemagus Never tried to update to master from the GitHub UI. Next time I'll git it a try and will do by myself. Thanks!

Ah! I will be aware of the resolution of PHPMD issue to remove the related de-duplication part of this PR when solved.

Arcanemagus · 2017-09-21T20:11:06Z

For anyone curious, the "default 10 seconds" I see? It's because virtually all the other projects are using jasmine-fix, and it sets the timeout to 10 seconds. Now that I know where it's coming from I can override it and revisit #74 😛.

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

Published in v0.3.0 🎉.

Arcanemagus suggested changes Apr 19, 2017

View reviewed changes

Arcanemagus added the enhancement label Apr 19, 2017

cgalvarez mentioned this pull request May 5, 2017

Let CodeClimate decide which engines to run #67

Merged

maxjacobson approved these changes May 30, 2017

View reviewed changes

fix duplicated issues and upgrade to linter v2

d0d3780

Merge branch 'master' into fix/duplicated-issues

4499d9b

Arcanemagus approved these changes Jun 12, 2017

View reviewed changes

Arcanemagus merged commit ea0adbc into AtomLinter:master Jun 12, 2017

cgalvarez mentioned this pull request Feb 26, 2018

Asyncify the specs #74

Closed

Fix duplicated issues and upgrade to linter v2 #65

Fix duplicated issues and upgrade to linter v2 #65

Conversation

cgalvarez commented Apr 19, 2017

cgalvarez commented Apr 19, 2017

Arcanemagus Apr 19, 2017

Choose a reason for hiding this comment

Arcanemagus Apr 19, 2017

Choose a reason for hiding this comment

cgalvarez Apr 19, 2017

Choose a reason for hiding this comment

Arcanemagus Apr 19, 2017

Choose a reason for hiding this comment

Arcanemagus commented Apr 19, 2017

cgalvarez commented Apr 19, 2017 • edited Loading

Arcanemagus commented Apr 19, 2017

cgalvarez commented Apr 19, 2017

Arcanemagus commented Apr 19, 2017

Arcanemagus Apr 19, 2017

Choose a reason for hiding this comment

cgalvarez Apr 20, 2017

Choose a reason for hiding this comment

Arcanemagus Apr 19, 2017

Choose a reason for hiding this comment

cgalvarez commented Apr 20, 2017

Arcanemagus commented Apr 20, 2017

cgalvarez commented Apr 20, 2017 • edited Loading

cgalvarez commented May 4, 2017

Arcanemagus commented May 4, 2017

cgalvarez commented May 5, 2017

pointlessone commented May 5, 2017

Arcanemagus commented May 5, 2017

Arcanemagus commented May 5, 2017

cgalvarez commented May 5, 2017

cgalvarez commented May 5, 2017

Arcanemagus commented May 5, 2017

pointlessone commented May 6, 2017

Arcanemagus commented May 7, 2017

cgalvarez commented May 9, 2017 • edited Loading

cgalvarez commented May 9, 2017

cgalvarez commented May 9, 2017

cgalvarez commented May 16, 2017

pointlessone commented May 16, 2017

cgalvarez commented May 16, 2017

Arcanemagus commented May 31, 2017

cgalvarez commented Jun 9, 2017

Arcanemagus commented Jun 12, 2017 • edited Loading

Arcanemagus commented Jun 12, 2017

Arcanemagus commented Jun 12, 2017

cgalvarez commented Jun 13, 2017

Arcanemagus commented Sep 21, 2017

Arcanemagus commented Apr 21, 2018

cgalvarez commented Apr 19, 2017 •

edited

Loading

cgalvarez commented Apr 20, 2017 •

edited

Loading

cgalvarez commented May 9, 2017 •

edited

Loading

Arcanemagus commented Jun 12, 2017 •

edited

Loading