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.

Sign up for GitHub

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

use consistent directory-separator in baseline #700

Merged
merged 18 commits into from
Oct 17, 2021
Merged

use consistent directory-separator in baseline #700

Show file tree
Hide file tree
Changes from 9 commits
Commits
Show all changes
18 commits
Select commit Hold shift + click to select a range
8882efb
use consistent directory-separator in baseline
staabm Oct 3, 2021
5f504a7
added testcoverage
clxmstaab Oct 3, 2021
2e32b8f
cs
clxmstaab Oct 3, 2021
78cc7ed
extracted FileHelper::normalizePath()
clxmstaab Oct 4, 2021
ce4d5ba
fix typo
clxmstaab Oct 4, 2021
2da4c28
de-duplicate
clxmstaab Oct 4, 2021
ea11427
typo
clxmstaab Oct 4, 2021
89ab33f
cs
clxmstaab Oct 4, 2021
3166e1c
cs
clxmstaab Oct 4, 2021
f0fe583
rewrote ternary
staabm Oct 4, 2021
ba20be5
Update src/AstRunner/AstMap/FileName.php
staabm Oct 4, 2021
f0930b6
Update FileName.php
staabm Oct 4, 2021
9f0fa6e
cs
staabm Oct 4, 2021
af96f19
Update FileName.php
staabm Oct 4, 2021
c14365c
Update FileName.php
staabm Oct 4, 2021
f4209e3
Update FileName.php
staabm Oct 4, 2021
f4caffd
typo
staabm Oct 4, 2021
3831943
cs
staabm Oct 4, 2021
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
8 changes: 6 additions & 2 deletions src/AstRunner/AstMap/FileName.php
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -4,20 +4,24 @@

namespace Qossmic\Deptrac\AstRunner\AstMap;

use Qossmic\Deptrac\File\FileHelper;

final class FileName implements TokenName
{
private string $path;

public function __construct(string $path)
{
$this->path = $path;
$this->path = FileHelper::normalizePath($path);
}

public function toString(): string
{
$wd = getcwd();

staabm marked this conversation as resolved.
Show resolved Hide resolved
return false !== $wd && 0 === strpos($this->path, $wd) ? substr($this->path, strlen($wd)) : $this->path;
$path = false !== $wd && 0 === strpos($this->path, $wd) ? substr($this->path, strlen($wd)) : $this->path;

return FileHelper::normalizePath($path);
Copy link
Collaborator

Choose a reason for hiding this comment

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

From what I see, the path is already normalized here.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

it doesn't really make a difference, but to compensate your comments I got rid of this ugly ternary and reduced the normalization to the very minimal

}

public function getFilepath(): string
Expand Down
5 changes: 2 additions & 3 deletions src/Collector/DirectoryCollector.php
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -6,6 +6,7 @@

use LogicException;
use Qossmic\Deptrac\AstRunner\AstMap;
use Qossmic\Deptrac\File\FileHelper;

class DirectoryCollector extends RegexCollector implements CollectorInterface
{
Expand All @@ -28,9 +29,7 @@ public function satisfy(

$filePath = $fileReference->getFilepath();
$validatedPattern = $this->getValidatedPattern($configuration);

// make paths/patterns cross-OS compatible
$normalizedPath = str_replace('\\', '/', $filePath);
$normalizedPath = FileHelper::normalizePath($filePath);

return 1 === preg_match($validatedPattern, $normalizedPath);
}
Expand Down
8 changes: 8 additions & 0 deletions src/File/FileHelper.php
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -41,4 +41,12 @@ public function toAbsolutePath(string $path): string

return $this->workingDirectory.'/'.$path;
}

/**
* make paths cross-OS compatible.
*/
public static function normalizePath(string $path): string
{
return str_replace('\\', '/', $path);
}
}
3 changes: 2 additions & 1 deletion src/PathNameFilterIterator.php
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -4,6 +4,7 @@

namespace Qossmic\Deptrac;

use Qossmic\Deptrac\File\FileHelper;
use SplFileInfo;
use Symfony\Component\Finder\Iterator\PathFilterIterator;

Expand All @@ -16,7 +17,7 @@ public function accept(): bool
$filename = $fileInfo->getPathname();

if ('\\' === \DIRECTORY_SEPARATOR) {
$filename = str_replace('\\', '/', $filename);
$filename = FileHelper::normalizePath($filename);
}

return $this->isAccepted($filename);
Expand Down
22 changes: 22 additions & 0 deletions tests/AstRunner/AstMap/FileNameTest.php
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -0,0 +1,22 @@
<?php

declare(strict_types=1);

namespace Tests\Qossmic\Deptrac\AstRunner\AstMap;

use PHPUnit\Framework\TestCase;
use Qossmic\Deptrac\AstRunner\AstMap;

final class FileNameTest extends TestCase
{
public function testPathNormalization(): void
{
$fileName = new AstMap\FileName('/path/to/file.php');
$this->assertSame('/path/to/file.php', $fileName->getFilepath());
Copy link
Collaborator

Choose a reason for hiding this comment

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

It would be nice if we could mock getcwd method call here.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

why? its not necessary for the current test.

it would only be necessary, in case you want to test the already existing functionality which was not changed by this PR

Copy link
Collaborator

Choose a reason for hiding this comment

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

You are right, it would help with test coverage. That is what I meant by "nice". Also, it makes the test fragile. (not really on GitHub but locally for sure). If I were to run the test in the path directory, it would fail mysteriously. It is a tiny thing, but it is IMHO good practice to mock out system dependencies, like the current working directory.

$this->assertSame('/path/to/file.php', $fileName->toString());

$fileName = new AstMap\FileName('\\path\\to\\file.php');
$this->assertSame('/path/to/file.php', $fileName->getFilepath());
$this->assertSame('/path/to/file.php', $fileName->toString());
}
}