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

Split static analysis workflow #51

Merged
merged 1 commit into from
Oct 9, 2024

Conversation

greg0ire
Copy link
Member

@greg0ire greg0ire commented Oct 8, 2024

This should allow downstream projects to better prepare for the Psalm removal: they can use the new phpstan workflow whenever they want to drop Psalm, and when all of them do, then we can drop the static analysis workflow form this repository.

Proof that it does not break the old workflow: greg0ire/collections#2
Proof that the new workflow works: greg0ire/collections#3

@greg0ire
Copy link
Member Author

greg0ire commented Oct 8, 2024

To be tagged as 5.2.0

@greg0ire greg0ire force-pushed the split-sa-workflow branch 2 times, most recently from 2940366 to 6571c9b Compare October 8, 2024 14:35
@greg0ire
Copy link
Member Author

greg0ire commented Oct 8, 2024

Note that I removed the workflow template so that this workflow no longer gets suggested when setting up new workflows through the github UI

strategy:
matrix:
php-version:
- "${{ inputs.php-version }}"
Copy link
Member

Choose a reason for hiding this comment

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

why are we even using a matrix here, with a single value in the matrix ?

Copy link
Member Author

Choose a reason for hiding this comment

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

IIRC it is to make the PHP version appear in the label of the job.

Copy link
Member

@stof stof Oct 8, 2024

Choose a reason for hiding this comment

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

you can use name: "PHPStan (php: ${{ inputs.php-version }})" to make that in a simpler way AFAIK (and with an explicit intent)

Copy link
Member Author

Choose a reason for hiding this comment

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

Ok, I'll try that!

Copy link
Member Author

Choose a reason for hiding this comment

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

@stof see #52

- name: "Run a static analysis with phpstan/phpstan"
run: "vendor/bin/phpstan analyse -v"
name: "PHPStan (deprecated in favor of phpstan.yml)"
uses: "./.github/workflows/phpstan.yml"
Copy link
Member

Choose a reason for hiding this comment

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

shouldn't this forward the inputs instead of using the defaults of the phpstan.yml workflow ?

Copy link
Member Author

Choose a reason for hiding this comment

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

Oh right, good catch!

@greg0ire greg0ire mentioned this pull request Oct 8, 2024
@SenseException SenseException mentioned this pull request Oct 8, 2024
This should allow downstream projects to better prepare for the Psalm
removal: they can use the new phpstan workflow whenever they want to
drop Psalm, and when all of them do, then we can drop the static
analysis workflow form this repository.
@greg0ire greg0ire merged commit a233747 into doctrine:main Oct 9, 2024
@greg0ire greg0ire deleted the split-sa-workflow branch October 9, 2024 07:45
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.

3 participants