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

Add missing private property definitions #143

Merged
merged 1 commit into from
Jul 18, 2019
Merged

Conversation

clue
Copy link
Member

@clue clue commented Jul 17, 2019

Spotted via phpstan. Missing property definitions means that PHP
automatically allocates new properties as public properties.

Refs #75 and #135

Spotted via phpstan. Missing property definitions means that PHP
automatically allocates new properties as public properties.
@clue clue added this to the v1.1.0 milestone Jul 17, 2019
@clue clue requested review from jsor and WyriHaximus July 17, 2019 19:40
@WyriHaximus
Copy link
Member

This raises a question though, should we include phpstan throughout our packages?

@clue
Copy link
Member Author

clue commented Jul 17, 2019

This raises a question though, should we include phpstan throughout our packages?

I agree that there is some value in this, but I'm not sure it's worth the effort (build matrix is quite big already). I'm open to discuss this in another issue 👍

@WyriHaximus
Copy link
Member

This raises a question though, should we include phpstan throughout our packages?

I agree that there is some value in this, but I'm not sure it's worth the effort (build matrix is quite big already). I'm open to discuss this in another issue

Another issue, or would a PR demonstrating it also be an option 😄

@clue
Copy link
Member Author

clue commented Jul 17, 2019

This raises a question though, should we include phpstan throughout our packages?

I agree that there is some value in this, but I'm not sure it's worth the effort (build matrix is quite big already). I'm open to discuss this in another issue

Another issue, or would a PR demonstrating it also be an option smile

I would say whatever works best for you (or anybody who feels like contributing) 👍 Besides a new tool for our CI setup, this discussion also involves new contribution guidelines (how to run tests and verify everything is "okay" prior to contributing etc.), evaluate what other Q/A tools may or may not make sense (phan etc.) and whatnot. Don't get me wrong, I'm a huge fan of static analysis (tools) and regularly use this as part of higher level applications, so I'm curious to see how much value it provides in these much lower level implementations.

@jsor jsor merged commit fdcd762 into reactphp:master Jul 18, 2019
@clue clue deleted the refs branch July 18, 2019 09:05
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.

3 participants