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

Phpstan lvl 1 #973

Open
wants to merge 3 commits into
base: master
Choose a base branch
from
Open

Conversation

rajmundtoth0
Copy link

Hi there,

I was wondering how big a task would be to use PHPstan on this package, so I started to resolve the level 1 issues.

I had to change a few things here and there, hope you like it!

Changes:

  • Factories: changed to the modern version.
  • PHPUnit had some configurations prevented to generate code coverage
  • Removed the Assert helper method and used assertSame instead of Assert()::assertArraySubset
  • Typehinted a few methods
  • Added PHPStan to the CI

Let me know what you think, I am happy to raise the PHPStan level further.

@willpower232
Copy link
Contributor

There has been a substantial clean up (and introduction of phpstan level 5) in https://github.com/owen-it/laravel-auditing/tree/v14-dev which hopefully I'll get some time to look at one day.

@rajmundtoth0
Copy link
Author

I see, after taking a quick look, my changes are aligning with the ones in the referenced branch. So merging this will simplify the other.

@parallels999
Copy link

this would make it more difficult to merge V13 with v14

I think there is no one who reviews v14, perhaps it would be better for this PR to be redirected to V14 and complete what is missing

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.

3 participants