-
-
Notifications
You must be signed in to change notification settings - Fork 1.3k
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
Upgrade phpstan and psalm #4412
Conversation
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.
Please retarget against 2.12.x
.
e9034c5
to
732c66c
Compare
28900a6
to
c07b200
Compare
composer.json
Outdated
"symfony/console": "^2.0.5|^3.0|^4.0|^5.0", | ||
"vimeo/psalm": "^3.17.2" | ||
"vimeo/psalm": "^3.18.2" |
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.
@morozov should we drop the ^
part here? that way we avoid discrepancies between local runs and CI (at the cost of a warning from Composer I think)
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.
The ^ doesn’t block later versions so why a warning?
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 checked and you are right, there is no warning. Maybe that changed, or I have faulty memory. That said, I was proposing to remove the ^
, that's when I thought you would get a warning, you won't get a warning with it that's for 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.
Maybe that changed, or I have faulty memory.
No, Composer would definitely trigger a warning in some cases if some dependency constraint was declared as a specific version.
should we drop the
^
part here?
Why now? The specific version is locked in the lock file which we still commit to Git. I believe it should be done separately from this specific upgrade.
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 wasn't require we do it now, but in general. Now would be as good a time as any.
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.
Now would be as good a time as any.
The way how we manage development dependencies is for the most part initiated by the work being done in #4386 which seems stuck. Once we decide to make a change, we'll make it for all dependencies at once.
This patch comes from an external contributor and just updates a dependency. I don't see much point in increasing the scope.
1ccd983
to
2ee4fae
Compare
Thanks, @simPod. |
Thanks for patience with reviews :) |
Summary
In order to fix issues related but not explicitly caused by #4403