Skip to content
This repository has been archived by the owner on Jun 11, 2020. It is now read-only.

Proposed Verdicts collection #4

Closed
wants to merge 1 commit into from

Conversation

logarytm
Copy link
Collaborator

@logarytm logarytm commented Dec 7, 2017

No description provided.

}

return true;
return count($this->verdicts->failing()) === 0;
Copy link
Owner

Choose a reason for hiding this comment

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

1: Literals should be on the left side of an expression, for the sake of redability.

$name
));
}

return true;
return $verdictsForField->failing()->count === 0;
Copy link
Owner

Choose a reason for hiding this comment

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

1

use Countable;
use IteratorAggregate;

class Verdicts implements Countable, IteratorAggregate
Copy link
Owner

Choose a reason for hiding this comment

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

Add basic phpDoc

return new static(array_filter($this->verdicts, $function));
}

public function reduce(callable $function, $initial = null)
Copy link
Owner

Choose a reason for hiding this comment

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

You forgot return type here

return $this->verdicts;
}

public function /* Countable */ count(): int
Copy link
Owner

Choose a reason for hiding this comment

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

I don't know, but is this PSR-2 compliant to have a comment here?

@Albert221
Copy link
Owner

Besides those issues, I personally don't like the name Verdicts, because it could be easily mistaken with Verdict. I'd propose VerdictList or VerdictCollection over it, prefering the second one.

Also, I'd like to change ValidationState API a little bit, but that's for its own issue.

@Albert221
Copy link
Owner

Closing as the verdicts branch has this continued

@Albert221 Albert221 closed this Dec 10, 2017
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

2 participants