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

Internal: CI code style and psalm #50

Open
kkmuffme opened this issue Dec 15, 2023 · 12 comments
Open

Internal: CI code style and psalm #50

kkmuffme opened this issue Dec 15, 2023 · 12 comments

Comments

@kkmuffme
Copy link
Collaborator

Add 2 tasks to the CI pipeline:

  1. run psalm on the plugin itself
  2. run codestyle check

See psalm's CI config, as that can be reused.

@paulshryock
Copy link

I would be willing to take a stab at this if it's helpful.

@kkmuffme
Copy link
Collaborator Author

@paulshryock that would be very much appreciated

The v5 branch is merged now, so if you could add this now, we could fix any issues psalm identifies in the plugin code and then release it finally.

@paulshryock
Copy link

paulshryock commented Dec 17, 2023

@kkmuffme sure thing. Want to assign this Issue to me, and I'll make one or more pull request(s) against the latest master branch?

@paulshryock
Copy link

So to start, I've forked this repo to paulshryock/psalm-plugin-wordpress. I cloned the repo locally, switched to PHP 8.3.0, ran composer install, and composer analyze.

I've got these warnings:

Warning: "findUnusedBaselineEntry" will default to "true" in Psalm 6. You should explicitly enable or disable this setting.
Warning: "findUnusedCode" will default to "true" in Psalm 6. You should explicitly enable or disable this setting.
Warning: "findUnusedBaselineEntry" will default to "true" in Psalm 6. You should explicitly enable or disable this setting.
Warning: "findUnusedCode" will default to "true" in Psalm 6. You should explicitly enable or disable this setting.

And the rest of the output from Psalm shows 29 errors and 19 other issues.

------------------------------
29 errors found
------------------------------
19 other issues found.

I will start by clearing up the errors, so once we add Psalm to the continuous integration pipeline, we have a passing pipeline out of the gate.

@kkmuffme
Copy link
Collaborator Author

Ok good, you need to create a psalm.xml and commit it to the repo so we get some consistency and also fix the errors above
Check psalm's own (I think psalm.xml.dist), but should be something like:

<?xml version="1.0"?>
<psalm
	xmlns:xsi="http://www.w3.org/2001/XMLSchema-instance"
	xmlns="https://getpsalm.org/schema/config"
	xsi:schemaLocation="https://getpsalm.org/schema/config vendor/vimeo/psalm/config.xsd"
	name="psalm-plugin-wordpress"
	errorLevel="1"
	compressor="off"
	phpVersion="7.4"
	memoizeMethodCallResults="true"
	findUnusedVariablesAndParams="true"
	findUnusedPsalmSuppress="true"
	findUnusedBaselineEntry="true"
	findUnusedCode="true"
/>

Btw I used php 7.4 since that is the minimum PHP version psalm supports, so we should stick to that.
Ideally you would add a 2nd pipeline where you set PHP 8.3 to check for any deprecated stuff used.
(again see psalm's own pipeline matrix, they use PHP 8 - 8.3, but I think that's overkill for here)

@kkmuffme
Copy link
Collaborator Author

@paulshryock fyi all the "internal" errors you don't need to fix - in fact I suggest do suppress them via the psalm config file, as they're caused by the plugin's namespace prior from moving the plugin to psalm and are all false positives.

@paulshryock
Copy link

@paulshryock fyi all the "internal" errors you don't need to fix - in fact I suggest do suppress them via the psalm config file, as they're caused by the plugin's namespace prior from moving the plugin to psalm and are all false positives.

Good to know, thanks. I should have something in another day or two. Hope I'm not holding up the release.

@kkmuffme
Copy link
Collaborator Author

kkmuffme commented Jan 9, 2024

Any approx ETA for this?

@paulshryock
Copy link

paulshryock commented Jan 12, 2024

Hi @kkmuffme I'm sorry about the delay. The holidays took over and I didn't get to wrap this up before end of year like I hoped. I will try to pick this up over the weekend and get back to you next week.

@kkmuffme
Copy link
Collaborator Author

@paulshryock take your time, there's no rush. I released it like it is now to gather some more feedback and so we're more easily able to distinguish between bugs in v3 vs bugs added from code style.

@paulshryock
Copy link

❤️ Amazing, glad this isn't holding up the release. And thanks for understanding.

@paulshryock
Copy link

I'm sorry, I thought I would have time to complete this but I haven't, and I don't want to hold this up. I'm going to un-assign myself, in case anyone else wants to pick this up. Sorry for the delay without any movement. If my availability opens up and this is still open and unassigned, I might take another swing at it.

@paulshryock paulshryock removed their assignment Mar 28, 2024
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

No branches or pull requests

2 participants