WIP: Add PHPStanBear #1468

damngamerz · 2017-02-28T08:58:05Z

Introduce PHPStanBear
Checks the code with phpstan analyze.
Closes #1426

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-02-28T08:59:09Z

bears/php/PHPStanBear.py

+    LICENSE = 'AGPL-3.0'
+    CAN_DETECT = {'Syntax'}
+    @staticmethod
+    def create_arguments(filename, file, config_file):        


Line contains following spacing inconsistencies:

Trailing whitespaces.

SpaceConsistencyBear, severity NORMAL, section python.

The issue can be fixed by applying the following patch:

--- a/bears/php/PHPStanBear.py +++ b/bears/php/PHPStanBear.py @@ -16,5 +16,5 @@ LICENSE = 'AGPL-3.0' CAN_DETECT = {'Syntax'} @staticmethod - def create_arguments(filename, file, config_file): + def create_arguments(filename, file, config_file): return ('analyse', '--no-ansi', filename)

gitmate-bot · 2017-02-28T08:59:09Z

bears/php/PHPStanBear.py

+    LICENSE = 'AGPL-3.0'
+    CAN_DETECT = {'Syntax'}
+    @staticmethod
+    def create_arguments(filename, file, config_file):        


W291 trailing whitespace'

PycodestyleBear (W291), severity NORMAL, section autopep8.

gitmate-bot · 2017-02-28T08:59:10Z

bears/php/PHPStanBear.py

+    AUTHORS_EMAILS = {'[email protected]'}
+    LICENSE = 'AGPL-3.0'
+    CAN_DETECT = {'Syntax'}
+    @staticmethod


E301 expected 1 blank line, found 0'

PycodestyleBear (E301), severity NORMAL, section autopep8.

gitmate-bot · 2017-02-28T08:59:11Z

bears/php/PHPStanBear.py

+    AUTHORS = {'The coala developers'}
+    AUTHORS_EMAILS = {'[email protected]'}
+    LICENSE = 'AGPL-3.0'
+    CAN_DETECT = {'Syntax'}


The code does not comply to PEP8.

PEP8Bear, severity NORMAL, section autopep8.

The issue can be fixed by applying the following patch:

--- a/bears/php/PHPStanBear.py +++ b/bears/php/PHPStanBear.py @@ -15,6 +15,7 @@ AUTHORS_EMAILS = {'[email protected]'} LICENSE = 'AGPL-3.0' CAN_DETECT = {'Syntax'} + @staticmethod - def create_arguments(filename, file, config_file): + def create_arguments(filename, file, config_file): return ('analyse', '--no-ansi', filename)

damngamerz · 2017-02-28T09:01:14Z

The Bear works perfectly adding Screen shots for reference.

Results for tests.(I think I covered it all 😄 )

satwikkansal · 2017-02-28T11:26:03Z

bears/php/PHPStanBear.py

+
+    @staticmethod
+    def create_arguments(filename, file, config_file):
+        return ('analyse', '--no-ansi', filename)


PHPStan support different Rule levels as well. Please add arguments for rule_level as well. Check https://github.com/phpstan/phpstan#rule-levels

Also, you need to add the -c option followed by config_file for the configuration file to work.

@satwikkansal There's something wrong with the rule_level i have tried to add argument this way

def create_arguments(filename, file, config_file,phpstan_level: str='0'): return ('analyse', '--no-ansi','-l '+phpstan_level,filename)

now while executing coala over a bad_file

coala --bear-dirs=. --bears=PHPStanBear --files=hello-world.php -V --flush-cache [WARNING][20:20:01] The default coafile '.coafile' was not found. You can generate a configuration file with your current options by adding the `--save` flag or suppress any use of config files with `-I`. [DEBUG][20:20:01] Platform Linux -- Python 3.6.0, coalib 0.10.0.dev99999999999999 [DEBUG][20:20:01] The file cache was successfully flushed. Executing section cli... [DEBUG][20:20:01] 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][20:20:01] Files that will be checked: /home/damngamerz/bear/hello-world.php [DEBUG][20:20:01] Running bear PHPStanBear... [DEBUG][20:20:01] Running 'phpstan analyse --no-ansi -l 0 /home/damngamerz/bear/hello-world.php'

it doesn't catch any error.regex is fine cause earlier it was catching as you can see my screen shot which i have posted.
And similarly tried for -c argument for config-file

def create_arguments(filename, file, config_file,phpstan_config: str=os.devnull): return ('analyse', '--no-ansi','-c '+phpstan_config,filename)

same thing happens while running coala for this.

coala --bear-dirs=. --bears=PHPStanBear --files=hello-world.php -V --flush-cache [WARNING][20:40:56] The default coafile '.coafile' was not found. You can generate a configuration file with your current options by adding the `--save` flag or suppress any use of config files with `-I`. [DEBUG][20:40:56] Platform Linux -- Python 3.6.0, coalib 0.10.0.dev99999999999999 [DEBUG][20:40:56] The file cache was successfully flushed. Executing section cli... [DEBUG][20:40:56] 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][20:40:56] Files that will be checked: /home/damngamerz/bear/hello-world.php [DEBUG][20:40:56] Running bear PHPStanBear... [DEBUG][20:40:56] Running 'phpstan analyse --no-ansi -c /dev/null /home/damngamerz/bear/hello-world.php'

But i have noticed one thing, while invoking this following command in cli gives

phpstan analyse --no-ansi -c /dev/null /home/damngamerz/bear/hello-world.php Project config file at path /dev/null does not exist.

So for config-file im doing something wrong.Any Thoughts?

Project config file at path /dev/null does not exist

@damngamerz The error says that the path /dev/null which you provide as config file is not a valid path which is True as you've not create any configuration file yet.

Also regarding rule-level, try running phpstan directly with different levels and see if there is any output or if the regex changes. In your case, you've set the default level to 0 (which is the loosest), so I reckon that's why it wasn't able to capture your bad file errors.

@satwikkansal well then what can i do with -c so the phpstan can execute over the file without this error?
By default while running phpstan the level is set to 0.And i had ran that command in cli

phpstan analyse --no-ansi -l 0 /home/damngamerz/bear/hello-world.php ------ ----------------------------------------------------------------- Line hello-world.php ------ ----------------------------------------------------------------- 7 Syntax error, unexpected T_ECHO, expecting ';' or '{' on line 7 ------ ----------------------------------------------------------------- [ERROR] Found 1 error

Same error reproduced.

@damngamerz You must be having some bugs in your code that are causing this.
Please read our Developer's guide thoroughly as most of the things we generally need are mentioned over there, read the documentation of PHPStan to know how exactly their config file works.
And lastly, in case of some error feel free to discuss on the Gitter channel (as I might miss your notifications, others will still be able to help you out) 😉

satwikkansal · 2017-02-28T11:34:01Z

bears/php/PHPStanBear.py

+    AUTHORS = {'The coala developers'}
+    AUTHORS_EMAILS = {'[email protected]'}
+    LICENSE = 'AGPL-3.0'
+    CAN_DETECT = {'Syntax'}


For PHPStan's readme, it seems like it can probably detect Unused Code, 'Variable Misuse', etc. Make sure to add all labels that seem appropriate to CAN_DETECT set. You can refer bear class documentation for the labels ;)

Yup added the followingCAN_DETECT = {'Syntax','Unused Code','Variable Misuse','Undefined Element','Missing Import','Spelling'}
Will be updating soon.

satwikkansal · 2017-02-28T11:44:56Z

tests/php/PHPStanBearTest.py

+
+    def test_run(self):
+        # Test a file with errors and warnings
+        self.check_validity(


check_validity asserts if your bear yields any results for a particular check with a list of strings. However, IMO it is good to check the result itself as well. Try using LocalBearTestHelper.check_results to check the results given by your bears.

Also, not necessary, but you can use verify_local_bear to create good_files and bad_files (good_file is a file which your bear considers as non-style-violating and a bad_file is one which has at least one error/warning/info)

@satwikkansal Well I have used check_validity as one of the other bear test file PHPLintBearTest uses those two test files (i.e.test_files/phplint_test1.php & test_files/phplint_test1.php).So if we have the resources why not use them.Should i really change it to check_results ? Cause i have read in the docs we can use any of those 3 methods to test a bear.

@damngamerz
check_validity just checks if there is a result from the bear or not, to test what exactly the result should be, we need to use check_results.
Here are the docs

@satwikkansal cool i will update 👍 😄

Im changing to verify_local_bear as later i can append more tests easily 👍

gitmate-bot · 2017-02-28T20:34:23Z

bears/php/PHPStanBear.py

+        """
+        :param phpstan_config:
+            path to a custom configuration file.
+            When using a custom project config file, you have to pass the phpstan_level argument


E501 line too long (96 > 79 characters)'

PycodestyleBear (E501), severity NORMAL, section autopep8.

gitmate-bot · 2017-02-28T20:34:23Z

bears/php/PHPStanBear.py

+                  'Undefined Element', 'Missing Import', 'Spelling'}
+
+    @staticmethod
+    def create_arguments(filename, file, config_file, phpstan_level: str='0', phpstan_config: str=''):


E501 line too long (102 > 79 characters)'

PycodestyleBear (E501), severity NORMAL, section autopep8.

gitmate-bot · 2017-02-28T20:34:24Z

bears/php/PHPStanBear.py

+        """
+        :param phpstan_config:
+            path to a custom configuration file.
+            When using a custom project config file, you have to pass the phpstan_level argument


Line is longer than allowed. (96 > 79)

LineLengthBear, severity NORMAL, section linelength.

gitmate-bot · 2017-02-28T20:34:25Z

bears/php/PHPStanBear.py

+                  'Undefined Element', 'Missing Import', 'Spelling'}
+
+    @staticmethod
+    def create_arguments(filename, file, config_file, phpstan_level: str='0', phpstan_config: str=''):


Line is longer than allowed. (102 > 79)

LineLengthBear, severity NORMAL, section linelength.

Mixih · 2017-02-28T20:40:39Z

tests/php/PHPStanBearTest.py

+"""
+
+
+PHPStanBearTest = verify_local_bear(


We have been trying to increase test quality and we are trying to move away form verify_local_bear. Please check the bear output to make sure it is outputting the right error.

@Mixih uhhh earlier i was using check_validity then i moved towards verify_local_bear on the suggestion of @satwikkansal . 😰

@damngamerz Sorry didn't knew that regarding verify_local_bear 😞
btw we should still use check_results over check_validity ( @Mixih please correct me if I'm wrong 😅 )

@satwikkansal @Mixih Most of the bear tests which i have seen are using check_validity and verify_local_bear.Till now i haven't seen a bear test using check_results so I'm really not that much comfortable with it.Decide guys 😛

The last line in testing docs says,

Use check_results as much as possible to test your bears.

So using check_results won't harm because it is more rigorous than check_validity

@satwikkansal Sorry but i need a second opinion this time.I don't want my effort to go vain again.As i said im not that much comfortable with check_results. And I haven't found any particular bear test using it.Most of the bear tests use check_validity and verify_local_bear.If i have to roll back to check_validity i can do that.
@Mixih Any thoughts about it ?

weeeell, we've been wanting proper test classes for a while. Please do use check_results with a proper test class with a skiptest decorator. The old way needs to go as it allows bears to spontaneously break.

gitmate-bot · 2017-02-28T20:40:41Z

bears/php/PHPStanBear.py

+
+    @staticmethod
+    def create_arguments(filename, file, config_file,
+                        phpstan_level: str='0', phpstan_config: str=''):


E128 continuation line under-indented for visual indent'

PycodestyleBear (E128), severity NORMAL, section autopep8.

gitmate-bot · 2017-02-28T20:40:41Z

bears/php/PHPStanBear.py

+
+    @staticmethod
+    def create_arguments(filename, file, config_file,
+                        phpstan_level: str='0', phpstan_config: str=''):


The code does not comply to PEP8.

PEP8Bear, severity NORMAL, section autopep8.

The issue can be fixed by applying the following patch:

--- a/bears/php/PHPStanBear.py +++ b/bears/php/PHPStanBear.py @@ -21,7 +21,7 @@ @staticmethod def create_arguments(filename, file, config_file, - phpstan_level: str='0', phpstan_config: str=''): + phpstan_level: str='0', phpstan_config: str=''): """ :param phpstan_config: path to a custom configuration file.

satwikkansal · 2017-02-28T21:04:42Z

bears/php/PHPStanBear.py

+            When using a custom project config file,
+            you have to pass the phpstan_level argument
+            (default value 0 does not apply here).
+        :param phpstan_level:


When using a custom project config file, you have to pass the phpstan_level argument (default value 0 does not apply here).

If that's the case, please add check for this situation in the code so that it doesn't result in a runtime error.

@satwikkansal uhhh I knew someone will point this out 😄 well i was searching for a solution in coala API docs and existing bears.But i couldn't find any interrupt kind of function, which can warn the user about this.Is there any?
One thing which i can propose is that setting the default value of rule_level as 1 whenever phpstan_config is used.And add these details in the Docstring.

Don't know if something relevant exists in coala api , but maybe you can do something like

if phpstan_config and phpstan_level: # add -c and phpstan_config to arguments

I have initialized phpstan_level='0' as you can see, so that won't work.
Instead as i have suggested this works.Tested 👍

args = ('analyse',) if phpstan_config: phpstan_level='1' args += ('--level='+phpstan_level,'-c '+phpstan_config, filename,) else: args += ('--level='+phpstan_level, filename,) return args