bears/php: Add PHPMessDetectorBear #1486

yash-nisar · 2017-03-05T18:19:23Z

Closes #22

For short term contributors: we understand that getting your commits well
defined like we require is a hard task and takes some learning. If you
look to help without wanting to contribute long term there's no need
for you to learn this. Just drop us a message and we'll take care of brushing
up your stuff for merge!

Checklist

I read the commit guidelines and I've followed
them.
I ran coala over my code locally. (All commits have to pass
individually. It is not sufficient to have "fixup commits" on your PR,
our bot will still report the issues for the previous commit.) You will
likely receive a lot of bot comments and build failures if coala does not
pass on every single commit!

After you submit your pull request, DO NOT click the 'Update Branch' button.
When asked for a rebase, consult coala.io/rebase
instead.

Please consider helping us by reviewing other peoples pull requests as well:

pick up any PR at https://coala.io/review
review it (check https://coala.io/reviewing for more info)
if you are sure that it needs work, use cobot mark wip <URL> to get it out
of the review queue.

The more you review, the more your score will grow at coala.io and we will
review your PRs faster!

gitmate-bot · 2017-03-05T18:20:28Z

tests/php/PHPMessDetectorBearTest.py

+def load_testfile(name):
+    return open(get_testfile_path(name)).readlines()
+
+@generate_skip_decorator(PHPMessDetectorBear)


E302 expected 2 blank lines, found 1'

PycodestyleBear (E302), severity NORMAL, section autopep8.

yash-nisar · 2017-03-06T19:23:33Z

Since PHPMD depends on composer for its installation, we shall wait till https://gitlab.com/coala/package_manager/issues/34 gets merged.

gitmate-bot · 2017-03-14T04:39:56Z

Hey! This pull request hasn't been updated for a while :/ It would be nice if we could get this going again!

gitmate-bot · 2017-03-21T14:27:21Z

Hey! This pull request hasn't been updated for a while :/ It would be nice if we could get this going again!

gitmate-bot · 2017-03-28T13:06:58Z

Hey! This pull request hasn't been updated for a while :/ It would be nice if we could get this going again!

gitmate-bot · 2017-04-04T11:47:20Z

Hey! This pull request hasn't been updated for a while :/ It would be nice if we could get this going again!

gitmate-bot · 2017-04-04T11:47:21Z

Hey! This pull request hasn't been updated for a while :/ It would be nice if we could get this going again!

gitmate-bot · 2017-04-11T21:33:47Z

Hey! This pull request hasn't been updated for a while :/ It would be nice if we could get this going again!

SanketDG · 2017-04-12T04:56:20Z

@yash-nisar this can be worked on now?

yash-nisar · 2017-04-12T05:18:51Z

Since I had included it in my GSoC proposal, the plan was to do it during GSoC if at all I get selected but I'll complete this asap once I'm done with my exams. :)

gitmate-bot · 2017-04-16T14:12:49Z

.ci/deps.sh

+  curl -fsSL -o phpmd.phar http://static.phpmd.org/php/latest/phpmd.phar
+  sudo chmod +x phpmd.phar
+  sudo mv phpmd.phar ~/phpmd/phpmd
+fi


Line contains following spacing inconsistencies:

No newline at EOF.

SpaceConsistencyBear, severity NORMAL, section ci.

The issue can be fixed by applying the following patch:

--- a/.ci/deps.sh +++ b/.ci/deps.sh @@ -184,4 +184,4 @@ curl -fsSL -o phpmd.phar http://static.phpmd.org/php/latest/phpmd.phar sudo chmod +x phpmd.phar sudo mv phpmd.phar ~/phpmd/phpmd -fi+fi

yash-nisar · 2017-04-16T15:12:45Z

Aaaanddddd the tests pass again ! 🎉

yash-nisar · 2017-04-16T15:38:44Z

@jayvdb @Makman2 Should I wait till we have ComposerRequirement or should I enhance this PR ?

jayvdb

No need to wait for ComposerRequirement for this; it is one line that can be added at the end of the release cycle.

jayvdb · 2017-04-17T11:03:46Z

bears/php/PHPMessDetectorBear.py

+
+    @staticmethod
+    def create_arguments(filename, file, config_file,
+                         phpmd_ruleset: typed_list(str)=''):


this is wrong on many levels.

never use an empty string as a default unless that is a valid value. Use None.

and this is supposed to be a list, but you give a string as a default.

But, since it is a mandatory argument, simply do not provide a default and coala will do the work for you.

@jayvdb Do you mean something like phpmd_ruleset: list ?

jayvdb · 2017-04-17T11:06:44Z

bears/php/PHPMessDetectorBear.py

+            Available rulesets: cleancode, codesize, controversial, design,
+            naming, unusedcode.
+        """
+        if phpmd_ruleset == '':  # pragma: no cover


this will all be removed due to the above comment, ...

but never use # pragma: no cover to avoid writing additional tests. It should be used only for insanely difficulty chunks of code to reach with a test.

Removed it. :)

jayvdb · 2017-04-17T11:07:05Z

tests/php/phpmessdetector_test_files/codesize_violation.php

+        }
+    }
+}
+?>


no eol at eof

We should make coala do that. :P

why does coala not pick this up?

@SanketDG coala doesn't pick this up even when I run it individually on this file.
Command: coala -V -b SpaceConsistencyBear -f s.php --flush-cache
s.php is the following file without a newline at eof.

This is coala/coala#2006

jayvdb · 2017-04-17T11:07:19Z

tests/php/phpmessdetector_test_files/cleancode_violation.php

+        }
+    }
+}
+?>


no eol at eof

Rectified. :)

gitmate-bot · 2017-04-18T07:40:33Z

bears/php/PHPMessDetectorBear.py

@@ -0,0 +1,39 @@
+from coalib.settings.Setting import typed_list


This file contains unused source code.

PyUnusedCodeBear, severity NORMAL, section flakes.

The issue can be fixed by applying the following patch:

--- a/bears/php/PHPMessDetectorBear.py +++ b/bears/php/PHPMessDetectorBear.py @@ -1,4 +1,3 @@ -from coalib.settings.Setting import typed_list from coalib.bearlib.abstractions.Linter import linter from dependency_management.requirements.DistributionRequirement import \ DistributionRequirement

gitmate-bot · 2017-04-18T08:46:46Z

bears/php/PHPMessDetectorBear.py

@@ -0,0 +1,39 @@
+from coalib.settings.Setting import typed_list


This file contains unused source code.

PyUnusedCodeBear, severity NORMAL, section flakes.

The issue can be fixed by applying the following patch:

--- a/bears/php/PHPMessDetectorBear.py +++ b/bears/php/PHPMessDetectorBear.py @@ -1,4 +1,3 @@ -from coalib.settings.Setting import typed_list from coalib.bearlib.abstractions.Linter import linter from dependency_management.requirements.DistributionRequirement import \ DistributionRequirement

Why do I forget to run coala ?!!

jayvdb · 2017-04-18T09:57:07Z

bears/php/PHPMessDetectorBear.py

+                         phpmd_ruleset: list):
+        """
+        :param phpmd_ruleset:
+            A ruleset filename or a comma-separated string of rulesetfilenames.


"rulesetfilenames." - what is that?

-> comma-separated string of ruleset filenames and built-in ruleset names.

But really we should be using config_file for the filename, which would mean that this ruleset variable is used only for the built-in ruleset names, which makes it more easily verified. (and the _config variable could then be managed by coala/coala#4072)

This comes at a slight loss of functionality, as only one config file could be used (It looks like phpmd supports multiple) , but presumably XInclude can be used to avoid that limitation.

Okay so IIUC, we mention the rulesets in our config_file ? @jayvdb

phpmd -h gives this:

yash ~ phpmd -h Mandatory arguments: 1) A php source code filename or directory. Can be a comma-separated string 2) A report format 3) A ruleset filename or a comma-separated string of rulesetfilenames

So, this is where I picked the description from 😛

Is this workaround satisfactory ? @jayvdb

@staticmethod def create_arguments(filename, file, config_file, phpmd_ruleset: list): """ :param phpmd_ruleset: A ruleset filename or a comma-separated string of rulesetfilenames. Available rulesets: cleancode, codesize, controversial, design, naming, unusedcode. """ config_file = phpmd_ruleset return filename, 'text', ','.join(config_file)

So in any way it's a list, right? I think it just suffices to say

A list of rulesets to use for analysis. Available rulesets: ...

And then I would call this setting phpmd_rulesets (note the s)

Awesome. 🎉

jayvdb · 2017-04-18T09:58:37Z

bears/php/PHPMessDetectorBear.py

+
+    @staticmethod
+    def create_arguments(filename, file, config_file,
+                         phpmd_ruleset: list):


list(str) !

Getting a type error, TypeError: 'type' object is not iterable.

--> typed_list(str) is the right one ;)

Makman2 · 2017-04-18T15:39:51Z

bears/php/PHPMessDetectorBear.py

@@ -0,0 +1,38 @@
+from coalib.bearlib.abstractions.Linter import linter
+from dependency_management.requirements.DistributionRequirement import \
+    DistributionRequirement


braces instead of \ please ;)

Makman2 · 2017-04-18T15:40:46Z

bears/php/PHPMessDetectorBear.py

+
+@linter(executable='phpmd',
+        output_format='regex',
+        output_regex=r'(?P<filename>.*):(?P<line>\d+)(?P<message>.*)')


isn't there some space between line and message? Could you post me an output example of phpmd?

Sure, just a minute. :)

[WARNING][21:11:32] Implicit 'Default' section inheritance is deprecated. It will be removed soon. To silence this warning remove settings in the 'Default' section from your coafile. You can use dots to specify inheritance: the section 'all.python' will inherit all settings from 'all'. Please enter a value for the setting "phpmd_ruleset" (A ruleset filename or a comma-separated string of rulesetfilenames. Available rulesets: cleancode, codesize, controversial, design, naming, unusedcode.) needed by PHPMessDetectorBear for section "cli": cleancode [DEBUG][21:11:37] Platform Linux -- Python 3.5.2, coalib 0.11.0.dev99999999999999 [DEBUG][21:11:37] The file cache was successfully flushed. Executing section cli... [DEBUG][21:11:37] Files that will be checked: /home/yash/s.php [DEBUG][21:11:37] coala is run only on changed files, bears' log messages from previous runs may not appear. You may use the `--flush-cache` flag to see them. [DEBUG][21:11:37] Running bear PHPMessDetectorBear... [DEBUG][21:11:37] Running 'phpmd /home/yash/s.php text cleancode' s.php | 8| ········}·else·{ | | [NORMAL] PHPMessDetectorBear: | | The method bar uses an else expression. Else is never necessary and you can simplify the code to work without else. | | *0: Do nothing | | 1: Open file(s) | | 2: Add ignore comment | | Enter number (Ctrl-D to exit): 0```

This is the output of the phpmd tool:

yash (e) venv ~ phpmd /home/yash/s.php text cleancode /home/yash/s.php:8 The method bar uses an else expression. Else is never necessary and you can simplify the code to work without else.

Will update my regex to (?P<filename>.*):(?P<line>\d+)\s*(?P<message>.*), note that I've added an \s* to capture spaces. :)

Interesting though, looks like coala does strip the message. And now I recall that from somewhen... but still: better explicit than implicit 👍

Makman2 · 2017-04-18T15:44:39Z

bears/php/PHPMessDetectorBear.py

+            Available rulesets: cleancode, codesize, controversial, design,
+            naming, unusedcode.
+        """
+        return filename, 'text', ','.join(phpmd_ruleset)


what's text?

An available report format. It can be xml, text or html. :)

Makman2 · 2017-04-18T15:45:06Z

bears/php/PHPMessDetectorBear.py

+
+    @staticmethod
+    def create_arguments(filename, file, config_file,
+                         phpmd_ruleset: list):


Maybe we want to make this setting optional by kicking in all available rulesets by default? Not sure...

So IIUC, do we provide all the rulesets as default like phpmd_ruleset: list=[rule1, rule2, ...] ?

yeah exactly, though I'm really not sure if we wanna do that...

What are the current defaults of phpmd?

The ruleset is a mandatory argument, so phpmd s.php text yields nothing whereas phpmd s.php text cleancode yields results.

okay let's keep it mandatory for now^^

yash-nisar · 2017-04-22T05:31:27Z

Reviewers v1.0

Used braces instead of , addressing bears/php: Add PHPMessDetectorBear #1486 (comment).
Updated regex to (?P<filename>.*):(?P<line>\d+)\s*(?P<message>.*). Note the \s* that has been included, addressing bears/php: Add PHPMessDetectorBear #1486 (comment).
Changed list -> typed_list(str) in phpmd_rulesets: typed_list(str), addressing bears/php: Add PHPMessDetectorBear #1486 (comment).
Change phpmd_ruleset -> phpmd_rulesets, addressing bears/php: Add PHPMessDetectorBear #1486 (comment).
Added extra tests to include one test per ruleset.
@Makman2 Can you have a look when you're free ?

jayvdb · 2017-04-22T06:30:36Z

bears/php/PHPMessDetectorBear.py

+
+@linter(executable='phpmd',
+        output_format='regex',
+        output_regex=r'(?P<filename>.*):(?P<line>\d+)\s*(?P<message>.*)')


do not capture filename, as it is useless information that the bear already has. (please check our docs dont recommend capturing filename)

and don't use .* unless there is no other option.

Is everything else good ?

Closes coala#22

jayvdb · 2017-04-22T08:35:00Z

ack bb46ca3

jayvdb · 2017-04-22T08:35:10Z

@rultor merge

rultor · 2017-04-22T08:35:53Z

@rultor merge

@jayvdb OK, I'll try to merge now. You can check the progress of the merge here

rultor · 2017-04-22T08:37:19Z

@rultor merge

@jayvdb Done! FYI, the full log is here (took me 1min)

gitmate-bot added process/pending review size/L review carefully! labels Mar 5, 2017

gitmate-bot reviewed Mar 5, 2017

View reviewed changes

yash-nisar force-pushed the iss#22 branch from 383f5aa to efceb30 Compare March 5, 2017 18:23

yash-nisar changed the title ~~bears/php: Add PHPMessDetectorBear~~ WIP bears/php: Add PHPMessDetectorBear Mar 6, 2017

coala-bot added process/wip and removed process/pending review labels Mar 6, 2017

yash-nisar force-pushed the iss#22 branch 3 times, most recently from e0d4b9c to 13cb512 Compare April 16, 2017 14:11

gitmate-bot reviewed Apr 16, 2017

View reviewed changes

yash-nisar force-pushed the iss#22 branch 2 times, most recently from f7cd2bf to 95f0b60 Compare April 16, 2017 14:46

yash-nisar changed the title ~~WIP bears/php: Add PHPMessDetectorBear~~ bears/php: Add PHPMessDetectorBear Apr 17, 2017

yash-nisar force-pushed the iss#22 branch from 95f0b60 to a1ac5d5 Compare April 17, 2017 10:27

gitmate-bot added process/pending review and removed process/wip labels Apr 17, 2017

jayvdb requested changes Apr 17, 2017

View reviewed changes

gitmate-bot added the process/pending review label Apr 18, 2017

gitmate-bot reviewed Apr 18, 2017

View reviewed changes

yash-nisar force-pushed the iss#22 branch 2 times, most recently from ff7cc1d to cd2f965 Compare April 18, 2017 08:45

gitmate-bot reviewed Apr 18, 2017

View reviewed changes

yash-nisar force-pushed the iss#22 branch from cd2f965 to 77e7d8d Compare April 18, 2017 08:47

jayvdb requested changes Apr 18, 2017

View reviewed changes

jayvdb added process/wip and removed process/pending review labels Apr 18, 2017

Makman2 reviewed Apr 18, 2017

View reviewed changes

yash-nisar force-pushed the iss#22 branch from 77e7d8d to 4444da1 Compare April 22, 2017 05:04

gitmate-bot added process/pending review size/XL and removed process/wip size/L labels Apr 22, 2017

yash-nisar force-pushed the iss#22 branch from 4444da1 to 379317e Compare April 22, 2017 05:21

jayvdb requested changes Apr 22, 2017

View reviewed changes

bears/php: Add PHPMessDetectorBear

bb46ca3

Closes coala#22

yash-nisar force-pushed the iss#22 branch from 379317e to bb46ca3 Compare April 22, 2017 07:02

jayvdb approved these changes Apr 22, 2017

View reviewed changes

rultor merged commit bb46ca3 into coala:master Apr 22, 2017

yash-nisar deleted the iss#22 branch May 8, 2017 20:51

		@@ -0,0 +1,39 @@
		from coalib.settings.Setting import typed_list

bears/php: Add PHPMessDetectorBear #1486

bears/php: Add PHPMessDetectorBear #1486

Conversation

yash-nisar commented Mar 5, 2017 • edited

Checklist

Choose a reason for hiding this comment

yash-nisar commented Mar 6, 2017

gitmate-bot commented Mar 14, 2017

gitmate-bot commented Mar 21, 2017

gitmate-bot commented Mar 28, 2017

gitmate-bot commented Apr 4, 2017

gitmate-bot commented Apr 4, 2017

gitmate-bot commented Apr 11, 2017

SanketDG commented Apr 12, 2017

yash-nisar commented Apr 12, 2017 • edited

Choose a reason for hiding this comment

yash-nisar commented Apr 16, 2017

yash-nisar commented Apr 16, 2017

jayvdb 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

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

Makman2 Apr 18, 2017 • edited

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

yash-nisar commented Apr 22, 2017 • edited

Reviewers v1.0

Choose a reason for hiding this comment

Choose a reason for hiding this comment

jayvdb commented Apr 22, 2017

jayvdb commented Apr 22, 2017

rultor commented Apr 22, 2017

rultor commented Apr 22, 2017

yash-nisar commented Mar 5, 2017 •

edited

yash-nisar commented Apr 12, 2017 •

edited

Makman2 Apr 18, 2017 •

edited

yash-nisar commented Apr 22, 2017 •

edited