Separate file/directory/exists assertions #291
Merged
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
shadowhand
commented
Sep 21, 2023
| if (!\file_exists($value)) { | ||
| static::reportInvalidArgument(\sprintf( | ||
| $message ?: 'The file %s does not exist.', | ||
| $message ?: 'The path %s does not exist.', |
There was a problem hiding this comment.
This is technically incorrect, since file_exists is equivalent to:
return is_file($path) || is_dir($path);| @@ -1444,8 +1442,6 @@ public static function fileExists($value, $message = '') | |||
| */ | |||
| public static function file($value, $message = '') | |||
| { | |||
| static::fileExists($value, $message); | |||
There was a problem hiding this comment.
This is a pointless check.
| @@ -1462,11 +1458,9 @@ public static function file($value, $message = '') | |||
| */ | |||
| public static function directory($value, $message = '') | |||
| { | |||
| static::fileExists($value, $message); | |||
There was a problem hiding this comment.
This is a pointless check. If the directory does not exist, the next assertion will never run. If the directory does exist, then it duplicates work.
| if (!\is_dir($value)) { | ||
| static::reportInvalidArgument(\sprintf( | ||
| $message ?: 'The path %s is no directory.', | ||
| $message ?: 'The path %s is not a directory.', |
There was a problem hiding this comment.
Made this message consistent with Assert::file().
| @@ -1426,11 +1426,9 @@ public static function lengthBetween($value, $min, $max, $message = '') | |||
| */ | |||
| public static function fileExists($value, $message = '') | |||
| { | |||
| static::string($value); | |||
There was a problem hiding this comment.
I don't see other assertions doing this, so I removed it.
There is no need to use `file_exists` before using `is_file` and `is_dir`, this makes for confusing errors.
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.
There is no need to use
file_existsbefore usingis_fileandis_dir, this makes for confusing errors. For instance, if you callAssert::directory('invalid/path')then the reported error will be:Additionally, the messages for these assertions were either technically or grammatically incorrect.