search

owen-it / laravel-auditing

Record the change log from models in Laravel
https://laravel-auditing.com
MIT License
3.01k stars 387 forks source link

Test adding PHPStan to mergeflow #810

Closed MortenDHansen closed 1 year ago

MortenDHansen commented 1 year ago

see #808

MortenDHansen commented 1 year ago

@erikn69 This is one of those things where i am not sure why we are doing this.

  1. Is it important for our users? - and why?
  2. Is it right to add these changes to the current version?

I mean, i like types and all that, but it does not seem like our codebase is ready for this. ... or am i just too cautious.

erikn69 commented 1 year ago

Is it important for our users? - and why?

this is not visible to users, It is important to prevent possible bugs, to optimize the code, or to apply best practices

Is it right to add these changes to the current version?

it passes the tests correctly, It seems to me that there are no breaking changes, but I would feel safer for v14

but I think that it should not be put in the tests workflow, I am going to do a PR to show you

MortenDHansen commented 1 year ago

but I think that it should not be put in the tests workflow, I am going to do a PR to show you

I like that version better :)

MortenDHansen commented 1 year ago

@erikn69 - changed to v14 branch. If you approve, well make it so :)

erikn69 commented 1 year ago

Hi @MortenDHansen I noticed that the tests are not running, something broke or it is because it is the dev branch

MortenDHansen commented 1 year ago

Hi @MortenDHansen I noticed that the tests are not running, something broke or it is because it is the dev branch

Test only run on pull request. Edit: That was changed. They only run against master then :) https://github.com/owen-it/laravel-auditing/blob/74cccb458442541fb1176458d150569b71bf1aaa/.github/workflows/run-tests.yml#L3