Skip to content

Commit

Permalink
Apply suggestions from code review
Browse files Browse the repository at this point in the history 
Co-authored-by: Mostafa Khudair <[email protected]>
  • Loading branch information
@MGatner @mostafakhudair
MGatner and mostafakhudair authored May 24, 2021
1 parent 48c3cc2 commit d2adf4f
Show file tree
Hide file tree
Showing 3 changed files with 60 additions and 79 deletions.
4 changes: 2 additions & 2 deletions system/Language/en/Publisher.php
View file
Original file line number Diff line number Diff line change
Expand Up @@ -11,7 +11,7 @@

// Publisher language settings
return [
'expectedFile' => 'Publisher::{0} expects a valid file.',
'expectedDirectory' => 'Publisher::{0} expects a valid directory.',
'collision' => 'Publisher encountered an unexpected {0} while copying {1} to {2}.',
'expectedDirectory' => 'Publisher::{0} expects a valid directory.',
'expectedFile' => 'Publisher::{0} expects a valid file.',
];
10 changes: 5 additions & 5 deletions system/Publisher/Exceptions/PublisherException.php
View file
Original file line number Diff line number Diff line change
Expand Up @@ -15,6 +15,11 @@

class PublisherException extends FrameworkException
{
public static function forCollision(string $from, string $to)
{
return new static(lang('Publisher.collision', [filetype($to), $from, $to]));
}

public static function forExpectedDirectory(string $caller)
{
return new static(lang('Publisher.expectedDirectory', [$caller]));
Expand All @@ -24,9 +29,4 @@ public static function forExpectedFile(string $caller)
{
return new static(lang('Publisher.expectedFile', [$caller]));
}

public static function forCollision(string $from, string $to)
{
return new static(lang('Publisher.collision', [filetype($to), $from, $to]));
}
}
125 changes: 53 additions & 72 deletions system/Publisher/Publisher.php
View file
Original file line number Diff line number Diff line change
Expand Up @@ -20,24 +20,18 @@
/**
* Publisher Class
*
* Publishers read in file paths from a variety
* of sources and copy the files out to different
* destinations.
* This class acts both as a base for individual
* publication directives as well as the mode of
* discovery for said instances.
* In this class a "file" is a full path to a
* verified file while a "path" is relative to
* to its source or destination and may indicate
* either a file or directory fo unconfirmed
* existence.
* Class failures throw a PublisherException,
* but some underlying methods may percolate
* different exceptions, like FileException,
* FileNotFoundException, InvalidArgumentException.
* Write operations will catch all errors in the
* file-specific $errors property to minimize
* impact of partial batch operations.
* Publishers read in file paths from a variety of sources and copy
* the files out to different destinations. This class acts both as
* a base for individual publication directives as well as the mode
* of discovery for said instances. In this class a "file" is a full
* path to a verified file while a "path" is relative to its source
* or destination and may indicate either a file or directory of
* unconfirmed existence.
* class failures throw the PublisherException, but some underlying
* methods may percolate different exceptions, like FileException,
* FileNotFoundException or InvalidArgumentException.
* Write operations will catch all errors in the file-specific
* $errors property to minimize impact of partial batch operations.
*/
class Publisher
{
Expand Down Expand Up @@ -90,14 +84,17 @@ class Publisher
* Discovers and returns all Publishers
* in the specified namespace directory.
*
* @param string $directory
*
* @return self[]
*/
public static function discover($directory = 'Publishers'): array
public static function discover(string $directory = 'Publishers'): array
{
if (isset(self::$discovered[$directory]))
{
return self::$discovered[$directory];
}

self::$discovered[$directory] = [];

/** @var FileLocator $locator */
Expand Down Expand Up @@ -145,8 +142,7 @@ private static function resolveDirectory(string $directory): string
}

/**
* Resolves a full path and verifies
* it is an actual file.
* Resolves a full path and verifies it is an actual file.
*
* @param string $file
*
Expand Down Expand Up @@ -184,11 +180,10 @@ private static function filterFiles(array $files, string $directory): array
}

/**
* Returns any files whose basename matches
* the given pattern.
* Returns any files whose `basename` matches the given pattern.
*
* @param string[] $files
* @param string $pattern Regex or pseudo-regex string
* @param array<string> $files
* @param string $pattern Regex or pseudo-regex string
*
* @return string[]
*/
Expand Down Expand Up @@ -239,13 +234,12 @@ private static function wipeDirectory(string $directory)
}

/**
* Copies a file with directory creation
* and identical file awareness.
* Copies a file with directory creation and identical file awareness.
* Intentionally allows errors.
*
* @param string $from
* @param string $to
* @param bool $replace
* @param string $from
* @param string $to
* @param boolean $replace
*
* @return void
*
Expand Down Expand Up @@ -301,8 +295,7 @@ public function __construct(string $source = null, string $destination = null)
}

/**
* Cleans up any temporary files
* in the scratch space.
* Cleans up any temporary files in the scratch space.
*/
public function __destruct()
{
Expand All @@ -315,14 +308,13 @@ public function __destruct()
}

/**
* Reads in file sources and copies out
* the files to their destinations.
* This method should be reimplemented by
* child classes intended for discovery.
* Reads files in the sources and copies them out to their destinations.
* This method should be reimplemented by child classes intended for
* discovery.
*
* @return bool
* @return boolean
*/
public function publish()
public function publish(): bool
{
return $this->addPath('/')->merge(true);
}
Expand All @@ -347,8 +339,7 @@ protected function getScratch(): string
}

/**
* Returns any errors from the last
* write operation.
* Returns errors from the last write operation if any.
*
* @return array<string,Throwable>
*/
Expand All @@ -358,8 +349,7 @@ public function getErrors(): array
}

/**
* Optimizes and returns the
* current file list.
* Optimizes and returns the current file list.
*
* @return string[]
*/
Expand All @@ -372,11 +362,10 @@ public function getFiles(): array
}

/**
* Sets the file list directly.
* Files are still subject to verification.
* Sets the file list directly, files are still subject to verification.
* This works as a "reset" method with [].
*
* @param string[] $files A new file list to use
* @param string[] $files The new file list to use
*
* @return $this
*/
Expand Down Expand Up @@ -407,8 +396,7 @@ public function addFiles(array $files)
}

/**
* Verifies and adds a single file
* to the file list.
* Verifies and adds a single file to the file list.
*
* @param string $file
*
Expand Down Expand Up @@ -469,11 +457,10 @@ public function addDirectories(array $directories, bool $recursive = false)
}

/**
* Verifies and adds all files
* from a directory.
* Verifies and adds all files from a directory.
*
* @param string $directory
* @param bool $recursive
* @param string $directory
* @param boolean $recursive
*
* @return $this
*/
Expand Down Expand Up @@ -520,8 +507,8 @@ public function addPaths(array $paths, bool $recursive = true)
/**
* Adds a single path to the file list.
*
* @param string $path
* @param bool $recursive
* @param string $path
* @param boolean $recursive
*
* @return $this
*/
Expand Down Expand Up @@ -563,8 +550,7 @@ public function addUris(array $uris)
}

/**
* Downloads a file from the URI
* and adds it to the file list.
* Downloads a file from the URI, and adds it to the file list.
*
* @param string $uri Because HTTP\URI is stringable it will still be accepted
*
Expand All @@ -576,20 +562,19 @@ public function addUri(string $uri)
$file = $this->getScratch() . basename((new URI($uri))->getPath());

// Get the content and write it to the scratch space
$response = service('curlrequest')->get($uri);
write_file($file, $response->getBody());
write_file($file, service('curlrequest')->get($uri)->getBody());

return $this->addFile($file);
}

//--------------------------------------------------------------------

/**
* Removes any files from the list that match
* the supplied pattern (within the optional scope).
* Removes any files from the list that match the supplied pattern
* (within the optional scope).
*
* @param string $pattern Regex or pseudo-regex string
* @param string|null $scope A directory to limit the scope
* @param string $pattern Regex or pseudo-regex string
* @param string|null $scope The directory to limit the scope
*
* @return $this
*/
Expand All @@ -601,19 +586,17 @@ public function removePattern(string $pattern, string $scope = null)
}

// Start with all files or those in scope
$files = is_null($scope)
? $this->files
: self::filterFiles($this->files, $scope);
$files = is_null($scope) ? $this->files : self::filterFiles($this->files, $scope);

// Remove any files that match the pattern
return $this->removeFiles(self::matchFiles($files, $pattern));
}

/**
* Keeps only the files from the list that match
* the supplied pattern (within the optional scope).
* (within the optional scope).
*
* @param string $pattern Regex or pseudo-regex string
* @param string $pattern Regex or pseudo-regex string
* @param string|null $scope A directory to limit the scope
*
* @return $this
Expand Down Expand Up @@ -654,10 +637,9 @@ public function wipe()
//--------------------------------------------------------------------

/**
* Copies all files into the destination.
* Does not create directory structure.
* Copies all files into the destination, does not create directory structure.
*
* @param bool $replace Whether to overwrite existing files.
* @param boolean $replace Whether to overwrite existing files.
*
* @return boolean Whether all files were copied successfully
*/
Expand All @@ -684,10 +666,9 @@ public function copy(bool $replace = true): bool

/**
* Merges all files into the destination.
* Creates a mirrored directory structure
* only for files from source.
* Creates a mirrored directory structure only for files from source.
*
* @param bool $replace Whether to overwrite existing files.
* @param boolean $replace Whether to overwrite existing files.
*
* @return boolean Whether all files were copied successfully
*/
Expand Down

0 comments on commit d2adf4f

Please sign in to comment.