Skip to content
New issue

Have a question about this project? # for a free GitHub account to open an issue and contact its maintainers and the community.

#

By clicking “#”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? # to your account

Jump to bottom

psalm integration #59

Merged
merged 15 commits into from
May 18, 2021
Merged

psalm integration #59

merged 15 commits into from
May 18, 2021

Conversation

snapshotpl
Copy link
Member

Q A
QA yes

Description

Closes #53

boesing
boesing requested changes Jan 1, 2021
View reviewed changes
Copy link
Member

@boesing boesing left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I prefer having @var array<x,y> annotations over @psalm-var as these are valid in most IDEs as of now.
Afaik, list<x> is still not supported by most of the IDEs.

I know that @Ocramius has some opinions on that.

In my company, we are adding:

/**
 * @var array<int,string>
 * @psalm-var list<string>
 */

@Ocramius
Copy link
Member

Ocramius commented Jan 1, 2021

I prefer having @var array<x,y> annotations over @psalm-var as these are valid in most IDEs as of now.
Afaik, list<x> is still not supported by most of the IDEs.

If you want IDE support, you can add @var and @psalm-var in the same block.

Careful with removing list<T>! list<T> is a much stricter (better) type than array<int, T>, so if you really want IDE support, do not remove it, but add a @var block

Ocramius
Ocramius approved these changes Jan 1, 2021
View reviewed changes
Copy link
Member

@Ocramius Ocramius left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I think @boesing's suggestions cover most of what needs to be improved at type level: once that's through, 🚢

psalm.xml.dist Outdated
<InternalMethod>
<errorLevel type="suppress">
<referencedMethod name="PHPUnit\Framework\MockObject\Builder\InvocationMocker::method"/>
</errorLevel>
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I don't remember if this is copied from somewhere, but these <errorLevel blocks can be merged together 👍

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Also: not a blocker 👍

@weierophinney weierophinney added this to the 2.6.0 milestone Apr 13, 2021
@weierophinney
Copy link
Member

@snapshotpl I've rebased against current 2.6.x, which includes the cutover to GHA. The Psalm check is failing, and it looks like there's still some feedback that needs to be addressed; ping me when done, and I'll do a final review.

@lcobucci
Copy link
Member

Afaik, list is still not supported by most of the IDEs.

Everything works on PHPStorm 2021.1 here (both on @var and @param) 👍

@snapshotpl
Copy link
Member Author

@weierophinney done

boesing
boesing previously requested changes Apr 23, 2021
View reviewed changes
snapshotpl and others added 13 commits May 18, 2021 09:14
@snapshotpl @weierophinney
psalm integration
4b25d84 
Signed-off-by: Witold Wasiczko <witold@wasiczko.pl>
@snapshotpl @weierophinney
Add baseline
8cd5e84 
Signed-off-by: Witold Wasiczko <witold@wasiczko.pl>
@snapshotpl @weierophinney
Fix some issues found by psalm
f576011 
Signed-off-by: Witold Wasiczko <witold@wasiczko.pl>
@snapshotpl @weierophinney
Fix type
7dbd1d3 
Signed-off-by: Witold Wasiczko <witold@wasiczko.pl>
@snapshotpl @weierophinney
Add baseline
b2bdaf9 
Signed-off-by: Witold Wasiczko <witold@wasiczko.pl>
@snapshotpl @weierophinney
Fix some issues found by psalm
46f43c8 
Signed-off-by: Witold Wasiczko <witold@wasiczko.pl>
@snapshotpl @weierophinney
Fix type
161fb10 
Signed-off-by: Witold Wasiczko <witold@wasiczko.pl>
@snapshotpl @weierophinney
Fix psalm issues
5501af0 
Signed-off-by: Witold Wasiczko <witold@wasiczko.pl>
@snapshotpl @boesing @weierophinney
Apply suggestions from code review
5701e4c 
Co-authored-by: Maximilian Bösing <2189546+boesing@users.noreply.github.com>
Signed-off-by: Witold Wasiczko <witold@wasiczko.pl>
@snapshotpl @weierophinney
Improve psalm config
7b35915 
Signed-off-by: Witold Wasiczko <witold@wasiczko.pl>
@snapshotpl @weierophinney
Update baseline
5346737 
Signed-off-by: Witold Wasiczko <witold@wasiczko.pl>
@weierophinney
qa: incorporate feedback from @boesing
268a6eb 
- Revert addition of scalar type hint in `AbstractSerializer::serializeHeaders` to eliminate BC break
- Revert addition of void return type in `CallbackStream::seek` to eliminate BC break
- Revert parameter name change in `MessageTrait::hasHeader()` to eliminate BC break

Signed-off-by: Matthew Weier O'Phinney <matthew@weierophinney.net>
@weierophinney
fix: add parameter type hint annotations to CallbackStream::seek method
d839415 
Signed-off-by: Matthew Weier O'Phinney <matthew@weierophinney.net>
@weierophinney weierophinney dismissed boesing’s stale review May 18, 2021 14:31

I've incorporated the feedback and pushed changes

@weierophinney
qa: update phpunit config to latest schema
3c483b5 
Signed-off-by: Matthew Weier O'Phinney <matthew@weierophinney.net>
@weierophinney
qa: revert variable name change
119a9cf 
Signed-off-by: Matthew Weier O'Phinney <matthew@weierophinney.net>
@weierophinney weierophinney merged commit 7d20341 into laminas:2.6.x May 18, 2021
# for free to join this conversation on GitHub. Already have an account? # to comment
Reviewers

@weierophinney weierophinney weierophinney approved these changes

@Ocramius Ocramius Ocramius approved these changes

@boesing boesing boesing left review comments

Assignees
No one assigned
Labels
Enhancement
Projects
None yet
Milestone
2.6.0
Development

Successfully merging this pull request may close these issues.

Psalm integration
5 participants
@snapshotpl @Ocramius @weierophinney @lcobucci @boesing