Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Add Phpstan to CI #37

Merged
merged 2 commits into from
Sep 19, 2017
Merged

Add Phpstan to CI #37

merged 2 commits into from
Sep 19, 2017

Conversation

bendavies
Copy link
Contributor

Thoughts on level max?
Pretty small library, so not hard to make it pass on max.

@@ -37,6 +37,7 @@ class SerializableArrayObjectAsset extends ArrayObject implements Serializable
*/
public function __construct()
{
parent::__construct();
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Wouldn't add these

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

couldn't see a quick way to disable a single rule....
maybe i'm not groking the phpstan well enough

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

hmm, just have to use ignoreMessages?

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

yep

@@ -39,6 +39,7 @@ class PharExceptionAsset extends PharException
*/
public function __construct()
{
parent::__construct();
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Wouldn't add these

@@ -36,6 +36,7 @@ class PharAsset extends Phar
*/
public function __construct()
{
parent::__construct('foo');
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Wouldn't add these

@@ -36,6 +36,7 @@
*/
public function __construct()
{
parent::__construct();
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Wouldn't add these

@@ -36,6 +36,7 @@ class ExceptionAsset extends Exception
*/
public function __construct()
{
parent::__construct();
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Wouldn't add these

@@ -36,6 +36,7 @@ class ArrayObjectAsset extends ArrayObject
*/
public function __construct()
{
parent::__construct();
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Wouldn't add these

.travis.yml Outdated
script: ./vendor/bin/phpcs --standard=PSR2 ./src/ ./tests/
install: composer require "squizlabs/php_codesniffer:^3.0.2" "phpstan/phpstan:^0.9" --dev
script:
- ./vendor/bin/phpcs --standard=PSR2 ./src/ ./tests/
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Split these tools into separate steps: once is CS, the other is static analysis

@bendavies bendavies force-pushed the phpstan branch 2 times, most recently from 4781cec to 6369882 Compare September 19, 2017 12:22
@bendavies
Copy link
Contributor Author

@Ocramius done

Copy link
Member

@Ocramius Ocramius left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

LGTM 👍

@Ocramius
Copy link
Member

Thanks @bendavies!

@Ocramius Ocramius self-assigned this Sep 19, 2017
@Ocramius Ocramius added this to the 1.2.0 milestone Sep 19, 2017
@bendavies
Copy link
Contributor Author

@Ocramius think you forgot to hit merge :)

@Ocramius
Copy link
Member

Whoops!

@Ocramius Ocramius merged commit 7af8066 into doctrine:master Sep 19, 2017
@Majkl578
Copy link
Contributor

I wouldn't personally go with max (L7), it has some issues with union types etc., thus 5, maybe 6 at most.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants