Create Check_Result class #50
Conversation
jjgrainger
added
[Type] Enhancement
felixarntz
left a comment
felixarntz
left a comment
There was a problem hiding this comment.
@jjgrainger Left a few comments, mostly LGTM.
felixarntz
left a comment
felixarntz
left a comment
There was a problem hiding this comment.
@jjgrainger Production code LGTM, left some feedback on the tests.
| /** | ||
| * Check_Result instance. | ||
| * | ||
| * @var Check_Result | ||
| */ | ||
| protected $check_result; |
There was a problem hiding this comment.
FYI no need to do any formal doc blocks in tests. It's good to add comments when a test method does things that may not be immediately clear on why it does so, but generally tests don't need docs.
Obviously not a blocker, it's okay to have this. But not needed :)
mukeshpanchal27
left a comment
mukeshpanchal27
left a comment
There was a problem hiding this comment.
Thanks @jjgrainger The PR looks solid to me. I have only one suggestion regarding the count function.
Can we use the count function for errors and warnings? If yes, then it removes two variables from the class and the increment logic for those variables.
Ie.
public function get_error_count() {
return count( $this->errors );
}
cc. @felixarntz
|
Thanks @mukeshpanchal27 The structure of the errors and warnings arrays mean we cannot use the |
* Disallow functions: move_uploaded_file, passthru, proc_open * Adjust tests
Adds the Check_Result class
Closes #11