Fix duplicated issues and upgrade to linter v2

[cgalvarez]

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:

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

Here's a screenshot that shows the final appearance:

[cgalvarez]

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

[Arcanemagus]

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

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

[cgalvarez]

@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]

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]

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 :wink: and avoid bothering someone else with untested PRs.

[Arcanemagus]

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 😉.

[cgalvarez]

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! :clap:

All the requested changes have been commited.

[Arcanemagus]

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]

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]

@Arcanemagus Is there anything pending to merge this PR?

[Arcanemagus]

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]

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]

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

[Arcanemagus]

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]

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]

@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]

I'm not sure I always see 10 seconds :open_mouth:

Maybe you refer to the 10 seconds timeout for spawning a process with Helpers :smile: , 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]

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]

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

[Arcanemagus]

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

[cgalvarez]

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]

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]

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

[cgalvarez]

@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]

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]

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

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]

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:

1. Bringing in the atom-package-deps module (npm install --save atom-package-deps)
2. Specifying a minimum of linter v2 in package.json like this
3. 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]

@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]

@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]

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]

Thanks for sticking through with this @cgalvarez!

[cgalvarez]

@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]

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]

Published in v0.3.0 🎉.