Skip to content
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

various improvements for psalm #537

Merged
merged 1 commit into from
Feb 1, 2020
Merged

Conversation

orklah
Copy link
Contributor

@orklah orklah commented Feb 1, 2020

I fixed some errors, updated baseline.

It improved type coverage a bit.

Copy link
Member

@asgrim asgrim left a comment

Choose a reason for hiding this comment

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

LGTM, nice fixes, thank you!

@asgrim asgrim self-assigned this Feb 1, 2020
@asgrim asgrim added this to the 3.5.1 milestone Feb 1, 2020
@orklah
Copy link
Contributor Author

orklah commented Feb 1, 2020

oh. I missed the fact that node could be null in ReflectionFunctionAbstract because it can be unitialized.

I'll fix that

@orklah
Copy link
Contributor Author

orklah commented Feb 1, 2020

Mmh... when trying to add null to the list of types for node, Psalm highlight 18 other lines where it's not handled.

I could fix that for Psalm by adding assert($this->node !== null); on each method using it, but I'm not sure it's the best way to go... Any thoughts?

@asgrim
Copy link
Member

asgrim commented Feb 1, 2020

@orklah perhaps a private method that asserts the node is set, and update $this->node references to point to that method?

Copy link
Member

@asgrim asgrim left a comment

Choose a reason for hiding this comment

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

Nice! Looks good, waiting for build 👍

@orklah
Copy link
Contributor Author

orklah commented Feb 1, 2020

Nice! Looks good, waiting for build 👍

Please wait. I'm trying to reconcile getNode and getInitializedNode, they both do the same thing. I'm just running tests locally

@orklah
Copy link
Contributor Author

orklah commented Feb 1, 2020

It's done, it should pass CI.

Thanks for the advice!

@orklah
Copy link
Contributor Author

orklah commented Feb 1, 2020

fixed some CS issues and rebased

@Ocramius Ocramius modified the milestones: 3.5.1, 4.0.0 Feb 1, 2020
@Ocramius Ocramius merged commit 43afb69 into Roave:master Feb 1, 2020
@orklah orklah deleted the psalm-improvements branch February 1, 2020 12:23
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

3 participants