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 PHPStan Github action #110

Merged
merged 4 commits into from
Dec 13, 2019
Merged

Add PHPStan Github action #110

merged 4 commits into from
Dec 13, 2019

Conversation

rvanlaak
Copy link
Owner

I'm in favor of just merging this PR, and decide on fixing / adding baseline later.

@rvanlaak rvanlaak requested a review from Nyholm December 13, 2019 10:50
@Nyholm
Copy link
Collaborator

Nyholm commented Dec 13, 2019

Could you also add a baseline?

@rvanlaak
Copy link
Owner Author

Done, added baseline.

And learned something new Today. Apparently twig/twig should have been added as dev dependency, as PHPStan otherwise isn't able to parse that. In other words, PHPStan also helps with finding hidden missing dependencies 🎉

@Nyholm
Copy link
Collaborator

Nyholm commented Dec 13, 2019

Cool!
Thank you

@@ -0,0 +1,13 @@
name: Static code analysis

on: [push]
Copy link
Collaborator

Choose a reason for hiding this comment

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

Should be on push and pull_request.

Copy link
Owner Author

@rvanlaak rvanlaak Dec 13, 2019

Choose a reason for hiding this comment

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

I still didn't find a situation where solely push didn't run the tests. Adding them both on the contrary will run the tests twice when you open a PR.

What about leaving it for now? I'll file a PR if it causes problems 👍

Copy link
Collaborator

Choose a reason for hiding this comment

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

If you do a PR from a fork, then this will action will never run.

Feel free to merge this now and do a PR from a fork later to try yourself.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants