Skip to content

Conversation

@carlos-granados
Copy link
Contributor

Applies three new rules for PSalm level 6

$callable = $callee->getCallable();

if ($callee->isAnInstanceMethod()) {
if ($callee->isAnInstanceMethod() && is_array($callable)) {
Copy link
Contributor Author

Choose a reason for hiding this comment

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

Need to make sure that $callable is an array before using it as an array

parent::__construct($scopeName, $filterString, $callable, $description);

if ($this->isAnInstanceMethod()) {
if (is_array($callable)) {
Copy link
Contributor Author

Choose a reason for hiding this comment

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

Depending on the type of callable that we have we get the class name and method name using one way or another

private $firstBackgroundEnded = false;
/**
* @var Event[]
* @var array<array{0: Event, 1: string}>
Copy link
Contributor Author

Choose a reason for hiding this comment

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

Wrong doc comment

* Returns counters for different scenario result codes.
*
* @return array<TestResult::*, int>
* @return array<StepResult::*, int>
Copy link
Contributor Author

Choose a reason for hiding this comment

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

This includes one case that is defined in StepResult, so we use the derived class

stof
stof previously requested changes Dec 9, 2024
Copy link
Contributor

@acoulton acoulton left a comment

Choose a reason for hiding this comment

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

Looks good, just a small question/opportunity to extract some duplicate code if you'd like to do that

@acoulton
Copy link
Contributor

@carlos-granados it doesn't look like the branch is updated?

@carlos-granados
Copy link
Contributor Author

@acoulton argh, I committed the code but forgot to push it, done

Co-authored-by: Andrew Coulton <andrew@ingenerator.com>
Copy link
Contributor

@acoulton acoulton left a comment

Choose a reason for hiding this comment

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

Great :)

@carlos-granados
Copy link
Contributor Author

@stof happy with this now? I would like to merge it to continue with this upgrade. Thanks!

@carlos-granados carlos-granados merged commit 418a0f5 into Behat:master Dec 12, 2024
17 checks passed
@carlos-granados carlos-granados deleted the psalm-level-6-2 branch December 12, 2024 11:37
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.

4 participants