Chore/psalm #228
Merged
Chore/psalm #228
Conversation
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
A lot of these are false positives, because Psalm doesn't fully understand the way Whippet is structured.
These are definitely used, Psalm just doesn't understand how.
These are definitely used, it's not clear to me why Psalm can't see that.
It should be passing an array of the command and options, no a single string.
There is no namespaced version of the Exception class, so this should be referencing the top-level version.
Otherwise: a) There's a chance (though not following the current logic) it could be references when no value is set; b) Psalm complains about dynamic property
In key-value array loops.
This is defined in the bin file.
`$this->plugin_dir` will have been defined by the class using this trait by the time this method is used.
This is definitely used!
There is no need for the `die()`, as an `E_USER_ERROR` will stop execution of the script anyway.
From PHP 8.1 up, internal methods like `__toString()` that can be over-ridden require the over-riding method to declare a compatible return type. Failure to do so will result in a deprecation notice. However, currently, if we declare a return type on this method, it results in the Kahlan tests failing, as the `__toString()` method Kahlan generates on its double for this class does not have a return type, so is viewed as incompatible with the class it's trying to double. Declaring the ReturnTypeWillChange attribute prevents the deprecation notice being issued, and stops Psalm complaining about it. See: https://php.watch/versions/8.1/ReturnTypeWillChange
By default, Psalm will infer the PHP version it should be running against from the composer platform. We want to keep that composer platform at 7.4 for now, but for advance warning of potential PHP8 issues we also want to run Psalm against PHP8 versions, so we can use the `--php-version` option to specifically run Psalm against those PHP versions.
Before: we were trying to loop through `$this->plugins_locked` as if it was an array (as previously we'd been using `foreach` with key/value pairs). However, it's actually an object, and foreach-ing an object produces a key-value array of the visible properties of that object. Now: we use get_object_vars to get the visible properties as an array, then use array_keys on that.
snim2
reviewed
Oct 4, 2023
| @@ -81,7 +81,7 @@ public function clone_no_checkout($repository) | |||
| return false; | |||
| } | |||
|
|
|||
| $this->run_command("mv {$tmpdir}/.git {$this->repo_path}"); | |||
| $this->run_command(['mv', $tmpdir . '/.git', $this->repo_path]); | |||
There was a problem hiding this comment.
Non-blocking comment: there's a typo in the commit message here s/no/not/g
There was a problem hiding this comment.
Good spot! Fixed in a rebase I'll push post-approval.
snim2
reviewed
Oct 4, 2023
I've opened an issue for that here: #229 |
|
@snim2 This is ready for another look now. |
snim2
approved these changes
Oct 5, 2023
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
This PR adds Psalm and modifies the codebase to make it Psalm-compliant in PHP 7.4-8.2 inclusive.
I've modified the CI to explicitly run PHP in each of the PHP versions we run in that environment, because by default Psalm runs in the PHP version inferred from the composer platform (currently 7.4). That would mean, even though we run CI in 4 different PHP versions, Psalm would be running against 7.4 in all 4 pipelines. Now, it will run once against 7.4 (as part of the standard
script/testrun), but then also run against the PHP version set for that pipeline.Resolves #222