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

Added PHPStan and script to run "composer phpstan" #48

Merged
merged 2 commits into from
Sep 21, 2017

Conversation

holtkamp
Copy link
Contributor

@holtkamp holtkamp commented Sep 19, 2017

As suggested here: #28 (comment)

@holtkamp holtkamp changed the title Added PHPStan and script to run composer phpstan Added PHPStan and script to run "composer phpstan" Sep 19, 2017
@OGKevin OGKevin assigned OGKevin and dnl-blkv and unassigned OGKevin Sep 19, 2017
composer.json Outdated
},
"scripts": {
"phpstan": [
"./vendor/bin/phpstan analyse --level 5 src"
Copy link
Contributor

Choose a reason for hiding this comment

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

@holtkamp Why do we go exactly for level 5?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

no particular reason... as a bank you guys should probably "strive" for level 7 😄
https://github.com/phpstan/phpstan#rule-levels

Copy link
Contributor

Choose a reason for hiding this comment

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

@holtkamp what is the highest level the SDK is passing at the moment?

@holtkamp
Copy link
Contributor Author

Levels:

  • 7: 21 errors
  • 6: 16 errors
  • 5: 13 errors
  • 4: 7 errors
  • 3: 7 errors
  • 2: no errors

Enjoy the ride!

Copy link
Contributor

@dnl-blkv dnl-blkv left a comment

Choose a reason for hiding this comment

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

Good to merge :)

@dnl-blkv dnl-blkv merged commit d96585d into bunq:develop Sep 21, 2017
@holtkamp holtkamp deleted the patch-phpstan-support branch September 21, 2017 14:16
@holtkamp
Copy link
Contributor Author

adjust phpstan level

?? the purpose of a static analysis tool is not to reduce the level to pass the checks, but to fix the code...

@dnl-blkv
Copy link
Contributor

dnl-blkv commented Sep 21, 2017

@holtkamp you're absolutely right.

However, even though the code is not passing much, we are not merging anything broken. Now, we can increase the level. A PR increasing the level would include both the level number increase and the fixes required for the checks of the level to pass :).

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

Successfully merging this pull request may close these issues.

None yet

3 participants