search

laminas / laminas-di

Automated dependency injection for PSR-11 containers
https://docs.laminas.dev/laminas-di/
BSD 3-Clause "New" or "Revised" License
36 stars 20 forks source link

Add psalm static analysis, improve type inference #21

Closed tux-rampage closed 1 year ago

tux-rampage commented 3 years ago
Q A
Documentation no
Bugfix no
BC Break no
New Feature yes
RFC no
QA yes

Description

As decided by the technical steering committee, components should be covered by psalm static analysis. This PR attempts to replace phpstan with psalm.

It will also fix reported issues and provide support for templates/generics when possible.

tux-rampage commented 3 years ago

Note: Psalm cannot be installed as dependency with php 8 at the moment. See vimeo/psalm#4408

weierophinney commented 3 years ago

@tux-rampage I've rebased your branch based on current 3.3.x, which includes the switch from Travis to GHA for CI purposes. I've also renamed the psalm.xml file to psalm.xml.dist (for consistency with other repos) and ensured an up-to-date composer.lock is available (built using PHP 7.4).

CS checks and Psalm checks are currently failing, which you can now see in the checks list.

tux-rampage commented 2 years ago

Thanks for your reviews @boesing and @Ocramius I've addressed your feedback. Can you please re-check?

tux-rampage commented 2 years ago

@boesing Thanks for taking time to review,

Ooof, well this PR has really plenty of code changes and its not that easy to review. However, I've added a few things which should be addressed.

Yes, that's why I focused on adding types for the existing implementation without refactoring where possible. You pointed out a lot of good and valid points to improve.

I'd prefer to address the Improvements in separate smaller PRs and focus on the bugs you pointed out (like the incorrect string-asserts). As you noted, this one is already hard enough to review, and I don't want to add more complexity here as necessary. Would this be OK for you?

Ocramius commented 2 years ago

@tux-rampage a couple files require a rebase - nothing major, as we mostly only removed PHP 7.3 and locked everything with composer.lock.

I suggest moving most issues to Psalm's baseline and "closing the chapter" meanwhile :)

tux-rampage commented 2 years ago

@tux-rampage a couple files require a rebase - nothing major, as we mostly only removed PHP 7.3 and locked everything with composer.lock.

I suggest moving most issues to Psalm's baseline and "closing the chapter" meanwhile :)

Perfect agreed. I'd rather like to do this in smaller PRs that are easier to review.

Ocramius commented 1 year ago

After rebasing and many tiny adjustments (Psalm became MUCH more powerful over the last whole year, I decided that this is good to go as-is.

Thanks @tux-rampage, and sorry for dragging this on: very valuable work!

tux-rampage commented 1 year ago

After rebasing and many tiny adjustments (Psalm became MUCH more powerful over the last whole year, I decided that this is good to go as-is.

Thanks @tux-rampage, and sorry for dragging this on: very valuable work!

Thank you for finishing this. :+1: