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 Psalm to Sensei #7097

Merged
merged 5 commits into from
Aug 16, 2023
Merged

Add Psalm to Sensei #7097

merged 5 commits into from
Aug 16, 2023

Conversation

merkushin
Copy link
Member

@merkushin merkushin commented Aug 15, 2023

Resolves part of #7094

Proposed Changes

  • Add Psalm to Sensei

Testing Instructions

  1. Run composer install
  2. Run vendor/bin/psalm
  3. It is expected to get thousands of issues.
------------------------------
5385 errors found
------------------------------
8789 other issues found.
You can display them with --show-info=true
------------------------------

Pre-Merge Checklist

  • PR title and description contain sufficient detail and accurately describe the changes
  • Acceptance criteria is met
  • Decisions are publicly documented
  • Adheres to coding standards (PHP, JavaScript, CSS, HTML)
  • All strings are translatable (without concatenation, handles plurals)
  • Follows our naming conventions (P6rkRX-4oA-p2)
  • Hooks (p6rkRX-1uS-p2) and functions are documented
  • New UIs are responsive and use a mobile-first approach
  • New UIs match the designs
  • Different user privileges (admin, teacher, subscriber) are tested as appropriate
  • Code is tested on the minimum supported PHP and WordPress versions
  • User interface changes have been tested on the latest versions of Chrome, Firefox and Safari
  • "Needs Documentation" label is added if this change requires updates to documentation
  • Known issues are created as new GitHub issues

@merkushin merkushin added this to the 4.16.2 milestone Aug 15, 2023
@merkushin merkushin requested a review from a team August 15, 2023 12:51
@merkushin merkushin self-assigned this Aug 15, 2023
@codecov
Copy link

codecov bot commented Aug 15, 2023

Codecov Report

Merging #7097 (960109a) into trunk (709d6dc) will not change coverage.
Report is 16 commits behind head on trunk.
The diff coverage is n/a.

Impacted file tree graph

@@            Coverage Diff            @@
##              trunk    #7097   +/-   ##
=========================================
  Coverage     49.35%   49.35%           
  Complexity    10541    10541           
=========================================
  Files           575      575           
  Lines         44513    44513           
  Branches        402      402           
=========================================
  Hits          21968    21968           
  Misses        22218    22218           
  Partials        327      327           

Continue to review full report in Codecov by Sentry.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update 58f8745...960109a. Read the comment docs.

composer.json Outdated
Comment on lines 12 to 18
"php-stubs/wordpress-stubs": "^6.3",
"phpcompatibility/phpcompatibility-wp": "2.1.4",
"sirbrillig/phpcs-no-get-current-user": "1.1.0",
"sirbrillig/phpcs-variable-analysis": "2.11.16",
"squizlabs/php_codesniffer": "3.7.2",
"symfony/polyfill-php80": "1.16.0",
"vimeo/psalm": "^4.22",
Copy link
Member

Choose a reason for hiding this comment

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

Should we use fixed versions for the new packages?

Copy link
Member Author

Choose a reason for hiding this comment

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

For tools like this one, I don't see any reasons to prevent a minor or patch update.
At the same time, I'm fine if we want to fix the version.

Copy link
Member Author

Choose a reason for hiding this comment

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

Did it here: 40ef9dd

Copy link
Member

Choose a reason for hiding this comment

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

I was just curious if you know why we're using fixed versions for dev dependencies. I'm perfectly fine with using non-fixed versions for the dev dependencies. Feel free to revert the change if you like.

@merkushin merkushin marked this pull request as ready for review August 15, 2023 23:50
@m1r0
Copy link
Member

m1r0 commented Aug 16, 2023

Hm, I'm getting the following error when running vendor/bin/psalm:

./vendor/bin/psalm
Problem parsing /Users/m1r0/Projects/sensei/psalm.xml:
  Could not resolve config path to /Users/m1r0/Projects/sensei//vendor-bin

I don't have that folder locally and if I remove vendor-bin from psalm.xml it seems to be working. Any ideas on what to do about it?

@merkushin
Copy link
Member Author

@m1r0 I encountered this issue while adding a GitHub action, fixed this way here: b0806dc

I don't remember where it came from in my local env, to maybe I just need to delete it from config at all.

@m1r0
Copy link
Member

m1r0 commented Aug 16, 2023

Adding allowMissingFiles="true" sounds good to me. 👍 Could you add the fix to this branch as well to avoid confusion?

@merkushin
Copy link
Member Author

@m1r0 Updated PR: 960109a

Copy link
Member

@m1r0 m1r0 left a comment

Choose a reason for hiding this comment

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

A shiny new tool, yum! 😎

All works as described.

@merkushin merkushin merged commit b79882c into trunk Aug 16, 2023
22 checks passed
@merkushin merkushin deleted the add/psalm branch August 16, 2023 20:19
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.

2 participants