Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Adding file matchers #73

Open
wants to merge 11 commits into
base: master
Choose a base branch
from

Conversation

vasekbrychta
Copy link

I'm sending first draft of IsExistingDirectory and IsExistingFile matchers. Please review the code and let me know if this structure is ok for you. I'll then create the rest of file matchers in a similar fashion and I'll update this pull request.

Merge IsExistingDirectoryPathTest to IsExistingDirectoryTest
Merge IsExistingFilePathTest to IsExistingFileTest
@vasekbrychta
Copy link
Author

What do you think @aik099?

hamcrest/Hamcrest.php Show resolved Hide resolved
@@ -0,0 +1,76 @@
<?php

declare(strict_types=1);
Copy link
Member

Choose a reason for hiding this comment

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

Please remove this and don't use non-PHP 5.3+ (from composer.json) incompatible code.


I haven't seen this in any of the other files.

Copy link
Author

@vasekbrychta vasekbrychta Sep 6, 2021

Choose a reason for hiding this comment

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

Ok, I saw that the tests are running for PHP 7.0 and above, so I assumed that the first supported version is PHP 7.0. Will change the code to be compatible with PHP 5.3 (btw PHP 5.3 reached end of life on 14 Aug 2014)

Comment on lines 16 to 19
/** @var string */
private $ownDescription;
/** @var string */
private $failureDescription;
Copy link
Member

Choose a reason for hiding this comment

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

Please follow the code format (also in comments) of the main codebase.

P.S.
The same applies to other places of the code.


DocBlock style doesn't match the rest of the matchers. They either don't have DocBlocks or format them in a multi-line way.

Copy link
Author

Choose a reason for hiding this comment

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

I'm sorry, I was too focused on the functionality itself that I forgot to maintain the style. Is there any cookbook/style definition or should I just try to find a similar case in the codebase?

Copy link
Member

@aik099 aik099 Sep 10, 2021

Choose a reason for hiding this comment

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

I have no idea. The https://github.com/hamcrest/hamcrest-php/blob/master/CONTRIBUTING.md file mentions coding standard but doesn't tell what it is. I'm assuming it's PSR-2.

Anyway, it's usually best to copy/paste new files from parts of the existing file to ensure that the code style is the same.

Comment on lines 43 to 50
if ($this->isSafeType($item))
{
$this->describeFileMismatch($this->createSplFileInfoObjectFromPath($item), $mismatchDescription);
}
else
{
parent::describeMismatch($item, $mismatchDescription);
}
Copy link
Member

Choose a reason for hiding this comment

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

Code formatting. Generally, it's the best idea to configure IDE as per the coding standards of the project and mass-reformat any new code.

Copy link
Author

Choose a reason for hiding this comment

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

Where can I get the coding standards for this project?

Copy link
Member

Choose a reason for hiding this comment

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

I have no idea. The https://github.com/hamcrest/hamcrest-php/blob/master/CONTRIBUTING.md file mentions coding standard but doesn't tell what it is. I'm assuming it's PSR-2.

Anyway, it's usually best to copy/paste new files from parts of the existing file to ensure that the code style is the same.

parent::__construct('an existing directory', 'is not a directory');
}

protected function matchesFile(\SplFileInfo $file): bool
Copy link
Member

Choose a reason for hiding this comment

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

Non a PHP 5.3+ compatible code. Likely you have more places, like this.

P.S.
I wonder any none of the builds have failed. Ha-ha, they don't include PHP 5.x in even though it's supported according to composer.json.

I guess that issue needs to be fixed in a separate PR if you're up for it.

* Accepts only <code>\SplFileInfo</code> objects or <code>string</code> paths.
*
* @return \Hamcrest\File\IsExistingFile
* @factory
Copy link
Member

Choose a reason for hiding this comment

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

This is likely to fail when used by a generator mentioned above.

Copy link
Author

Choose a reason for hiding this comment

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

Why do you think it might fail? I used the generator and it worked correctly.

Copy link
Member

Choose a reason for hiding this comment

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

Because I saw @factory annotation elsewhere with a parameter (e.g. @factory la-la) and assumed (haven't checked with generator code), that generator might fail.

tests/Hamcrest/File/IsExistingFileTest.php Outdated Show resolved Hide resolved
@vasekbrychta vasekbrychta marked this pull request as ready for review June 6, 2022 13:56
@vasekbrychta
Copy link
Author

Took me a bit longer, but it should be finally done.

I haven't included a matcher for absolute path because there's no easy way to get the absolute path that wouldn't be canonical in php.

Comment on lines +289 to +304
* `anEmptyFile` - evaluates to true if the file is empty
```php
$file = new \SplFileInfo('/var/log/php-fpm.log');
assertThat($file, anEmptyFile());

assertThat('/var/log/php-fpm.log', anEmptyFile());
```

* `aNonEmptyFile` - evaluates to true if the file is not empty
```php
$file = new \SplFileInfo('/var/log/php-fpm.log');
assertThat($file, aNonEmptyFile());

assertThat('/var/log/php-fpm.log', aNonEmptyFile());
```

Copy link
Member

Choose a reason for hiding this comment

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

Are you sure, that such matches exist in the Java version of the library?

Comment on lines +305 to +324
* `aFileNamed` - evaluates to true if the file name is equal to given value or the provided matcher matches the file name
```php
$file = new \SplFileInfo('/var/log/php-fpm.log');
assertThat($file, aFileNamed('php-fpm.log'));
assertThat($file, aFileNamed(startsWith('php')));

assertThat('/var/log/php-fpm.log', aFileNamed('php-fpm.log'));
assertThat('/var/log/php-fpm.log', aFileNamed(startsWith('php')));
```

* `aFileWithCanonicalPath` - evaluates to true if the file canonical path is equal to given value or the provided matcher matches the canonical path of the file
```php
$file = new \SplFileInfo('/var/log/./php-fpm.log');
assertThat($file, aFileWithCanonicalPath('/var/log/php-fpm.log'));
assertThat($file, aFileWithCanonicalPath(endsWith('log/php-fpm.log')));

assertThat('/var/log/./php-fpm.log', aFileWithCanonicalPath('/var/log/php-fpm.log'));
assertThat('/var/log/./php-fpm.log', aFileWithCanonicalPath(endsWith('log/php-fpm.log')));
```

Copy link
Member

Choose a reason for hiding this comment

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

In the Java version of the library, these matchers only accept another matcher as an argument but don't accept file reference (string or SPL).

assertThat('/var/log/./php-fpm.log', aFileWithCanonicalPath('/var/log/php-fpm.log'));
assertThat('/var/log/./php-fpm.log', aFileWithCanonicalPath(endsWith('log/php-fpm.log')));
```

### Object
Copy link
Member

Choose a reason for hiding this comment

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

The aFileWithAbsolutePath matcher wasn't implemented.

Comment on lines +54 to +58
public function describeMismatch($item, Description $description)
{
$this->_matcher->describeMismatch($item, $description);
}

Copy link
Member

Choose a reason for hiding this comment

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

Why this is needed? All existing matches seem to work without this.


/**
* Does file name satisfy a given matcher?
* Accepts only <code>\SplFileInfo</code> objects or <code>string</code> paths.
Copy link
Member

Choose a reason for hiding this comment

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

Wrong indentation.

public function testMatchesAFileWithEqualSize()
{
$this->assertMatches(
aFileWithSize(self::$ACTUAL_SIZE),
Copy link
Member

Choose a reason for hiding this comment

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

Not sure, that it's a good idea to use matcher functions from a global namespace vs namespaced methods.

Please see how it's implemented in other matchers.


protected function createMatcher()
{
return IsFileWithSize::aFileWithSize(not(anything()));
Copy link
Member

Choose a reason for hiding this comment

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

Something isn't right here. Should the createMatcher method have an argument given to the matcher and then all tests in this class create matcher via the createMatcher method?

Please see how it's implemented in other matcher tests, that have arguments.

Comment on lines +28 to +33
self::$EMPTY_FILE = new class('/tmp/empty') extends \SplFileInfo {
public function getSize()
{
return 0;
}
};
Copy link
Member

Choose a reason for hiding this comment

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

Why not create an empty file in the temp folder in the setUp method and then delete it in the tearDown method?

This way you can actually use that file name in tests instead of only using the SplFileInfo object.


self::$READABLE_FILE = new \SplFileInfo(self::$READABLE_FILE_PATH);

self::$NON_READABLE_FILE = new class('/tmp/non-readable') extends \SplFileInfo {
Copy link
Member

Choose a reason for hiding this comment

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

  1. What kind of class instantiation syntax is this?
  2. Does it create an anonymous class instance?
  3. Will this code work in all supported by this library PHP versions?


self::$WRITABLE_FILE = new \SplFileInfo(self::$WRITABLE_FILE_PATH);

self::$NON_WRITABLE_FILE = new class('/tmp/non-writable') extends \SplFileInfo {
Copy link
Member

Choose a reason for hiding this comment

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

  1. What kind of class instantiation syntax is this?
  2. Does it create an anonymous class instance?
  3. Will this code work in all supported by this library PHP versions?

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.

2 participants