-
Notifications
You must be signed in to change notification settings - Fork 6
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 vimeo/psalm, update to phpunit 7.x, change require-dev to require… #22
base: master
Are you sure you want to change the base?
Conversation
can you upgrade travis configuration to php7.4 so the test suit will run? |
possibly done .... (I'm commenting before I've checked if travis is happy ... it may not like php7.2 or 5.6 now, I'm not sure) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
i prefer to drop the vimeo/psalm support, i think that less dependency is better
.travis.yml
Outdated
@@ -1,9 +1,8 @@ | |||
language: php | |||
php: | |||
- 7.4 | |||
- 7.2 | |||
- 5.6 |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
can i also ask to drop php5.6 from travis-ci?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
done
composer.json
Outdated
"mikey179/vfsstream": "1.4.*" | ||
"php": ">=7.2", | ||
"phpunit/phpunit": "~7", | ||
"mikey179/vfsstream": "1.4.*", |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
can you upgrade to the latest 1.6.*?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
done (changed to ^1.4)
"php": ">=7.2", | ||
"phpunit/phpunit": "~7", | ||
"mikey179/vfsstream": "1.4.*", | ||
"vimeo/psalm" : "*" |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
can you drop this dependency?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
psalm is a dev only dependency - it won't affect users of the package.
it will help make sure you've not added errors into the codebase (E.g. wrong parameters, missing parameter, incorrect doc types etc).
"vimeo/psalm" : "*" | ||
}, | ||
"scripts": { | ||
"psalm" : "@php vendor/bin/psalm", |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
can you drop this script?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
adding it helps the code quality. I don't see why you'd want to drop it.
Normal users of the package will not install psalm as a dependency.
@@ -0,0 +1,15 @@ | |||
<?xml version="1.0"?> |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
can you drop psalm support? i think that is better to have less..
Upgrade to newer PHPUnit version
Add vimeo/psalm for possible static analysis checks