Skip to content

Updated contribution guide with "Automated Feedback" #15024

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

Merged
merged 1 commit into from
Feb 26, 2021

Conversation

Nyholm
Copy link
Member

@Nyholm Nyholm commented Feb 24, 2021

Adding a note about psalm.

Related PR: symfony/symfony#40291

@Nyholm
Copy link
Member Author

Nyholm commented Feb 24, 2021

Thank you

@wouterj wouterj added the Waiting Code Merge Docs for features pending to be merged label Feb 24, 2021
@carsonbot carsonbot added this to the next milestone Feb 24, 2021
@Nyholm Nyholm force-pushed the automated-feedback branch from 39f1794 to ffc79c0 Compare February 24, 2021 17:18
nicolas-grekas added a commit to symfony/symfony that referenced this pull request Feb 25, 2021
This PR was squashed before being merged into the 4.4 branch.

Discussion
----------

Adding a Github action to run Psalm

| Q             | A
| ------------- | ---
| Branch?       | 4.4
| Bug fix?      | no
| New feature?  | no
| Deprecations? | no
| Tickets       |
| License       | MIT
| Doc PR        | symfony/symfony-docs#15024

I've seen sometimes that we've forgotten to add `\` before `Throwable` or that we refer to a class that does not exist. One could argue that the code is not properly tested, but somehow these PRs still get merged. (And quickly fixed in a follow up PR).

I suggest to add psalm to check every PR for some errors that can be found with a static analyser. This is to help/automate the PR review process. All psalm errors found should be reviewed and discussed. The maintainers can decide to ignore some warnings if they want to. (Ie false positives)

This PR is about “Psalm PR review”. It does not try to fix “Psalm compatibility”. Psalm compatibility is a separate issue that should be discussed separate from the "Psalm PR review".

I currently plan to follow up with the more controversial topic of "Should we make Symfony more compatible with Psalm or not".

Commits
-------

c5ed24d Adding a Github action to run Psalm
@Nyholm
Copy link
Member Author

Nyholm commented Feb 25, 2021

The related PR is merged.

@javiereguiluz javiereguiluz removed the Waiting Code Merge Docs for features pending to be merged label Feb 26, 2021
@javiereguiluz javiereguiluz modified the milestones: next, 4.4 Feb 26, 2021
@javiereguiluz
Copy link
Member

Thank you Tobias! We merged this in 4.4 and up.

@javiereguiluz javiereguiluz merged commit d375f44 into symfony:4.4 Feb 26, 2021
@Nyholm Nyholm deleted the automated-feedback branch April 2, 2021 10:38
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.

6 participants