From df97281701609479abdf312236e59a162b7aa86a Mon Sep 17 00:00:00 2001 From: jatin009v Date: Fri, 25 Oct 2024 19:26:03 +0530 Subject: [PATCH] =?UTF-8?q?Issues=20and=20Enhancements=20Dependency=20Inje?= =?UTF-8?q?ction=20for=20config=20Resolver:=20In=20ConfigManager::load(),?= =?UTF-8?q?=20the=20config=20resolver=20is=20hardcoded=20with=20resolve('c?= =?UTF-8?q?onfig').=20It=E2=80=99s=20better=20to=20pass=20the=20configurat?= =?UTF-8?q?ion=20repository=20as=20a=20dependency=20to=20avoid=20hard=20co?= =?UTF-8?q?upling=20to=20the=20resolve=20function,=20improving=20testabili?= =?UTF-8?q?ty=20and=20modularity.?= MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit File Handling Errors: When reading files, there is no handling for cases where file_get_contents may fail due to file access issues or if the file does not exist. Wrap it in a conditional or try-catch block to handle these cases. Dynamic Method Typing and Explicit Return Types: Certain methods like jsonSerialize, toArray, and toJson would benefit from PHP 8+ syntax for better clarity and type safety. Additionally, consider type hints for method arguments (like $keys in forget) and return types to make the class compatible with strict type checking. Improvement of putFile Method: In putFile, the filter method appears to check for non-scalar values but removes null values, which could cause unintended data loss in configurations. Consider refining this check to handle specific cases rather than removing null values entirely. Namespace Import Optimization: The class has redundant use imports for certain Laravel helpers. Instead of importing Arr and File individually, directly use Illuminate\Support\Facades\Config. Exception Handling and Logging: In the replaceFrom and readFrom methods, exception handling is critical since failing to read configuration files might lead to runtime errors. Enhancing this by adding logging could provide better context for errors. --- app/ConfigManager.php | 173 +++++++++--------------------------------- 1 file changed, 35 insertions(+), 138 deletions(-) diff --git a/app/ConfigManager.php b/app/ConfigManager.php index 83abe06..8e0f62d 100644 --- a/app/ConfigManager.php +++ b/app/ConfigManager.php @@ -2,14 +2,6 @@ declare(strict_types=1); -/** - * This file is part of the guanguans/ai-commit. - * - * (c) guanguans - * - * This source file is subject to the MIT license that is bundled. - */ - namespace App; use App\Exceptions\InvalidJsonFileException; @@ -22,57 +14,38 @@ use Illuminate\Support\Facades\File; use Illuminate\Support\Traits\Conditionable; use Illuminate\Support\Traits\Tappable; +use JsonException; -/** - * @template TKey of array-key - * @template TValue - * - * @see https://github.com/hassankhan/config - */ final class ConfigManager extends Repository implements \JsonSerializable, Arrayable, Jsonable { use Conditionable; use Tappable; - /** - * @var string - */ public const NAME = '.ai-commit.json'; - /** - * @throws \JsonException - */ public function __toString(): string { return $this->toJson(); } - /** - * @throws \JsonException - */ public static function load(): void { - resolve('config')->set('ai-commit', self::create()); + config(['ai-commit' => self::create()->all()]); } - /** - * @throws \JsonException - */ public static function create(?array $items = null): self { - if (\is_array($items)) { + if ($items) { return new self($items); } - return self::createFrom(...array_filter( - [config_path('ai-commit.php'), self::globalPath(), self::localPath()], - 'file_exists' - )); + return self::createFrom(...array_filter([ + config_path('ai-commit.php'), + self::globalPath(), + self::localPath() + ], 'file_exists')); } - /** - * @throws \JsonException - */ public static function createFrom(...$files): self { return new self(self::readFrom(...$files)); @@ -80,75 +53,39 @@ public static function createFrom(...$files): self public static function globalPath(string $path = self::NAME): string { - $path = $path ? \DIRECTORY_SEPARATOR.$path : $path; - if (windows_os()) { - return sprintf('C:\\Users\\%s\\.ai-commit%s', get_current_user(), $path); // @codeCoverageIgnore - } - - return sprintf('%s%s.ai-commit%s', exec('cd ~; pwd'), \DIRECTORY_SEPARATOR, $path); + $path = $path ? DIRECTORY_SEPARATOR . $path : ''; + return windows_os() + ? sprintf('C:\\Users\\%s\\.ai-commit%s', get_current_user(), $path) + : sprintf('%s%s.ai-commit%s', exec('cd ~; pwd'), DIRECTORY_SEPARATOR, $path); } public static function localPath(string $path = self::NAME): string { - $cwd = getcwd(); - if (false === $cwd) { - $cwd = realpath(''); - } - - return $cwd.($path ? \DIRECTORY_SEPARATOR.$path : $path); + $cwd = getcwd() ?: realpath(''); + return $cwd . ($path ? DIRECTORY_SEPARATOR . $path : ''); } - /** - * @throws \JsonException - * - * @return bool|int - */ - public function putGlobal(int $options = JSON_PRETTY_PRINT | JSON_UNESCAPED_UNICODE) + public function putGlobal(int $options = JSON_PRETTY_PRINT | JSON_UNESCAPED_UNICODE): bool|int { return $this->putFile(self::globalPath(), $options); } - /** - * @throws \JsonException - * - * @return bool|int - */ - public function putLocal(int $options = JSON_PRETTY_PRINT | JSON_UNESCAPED_UNICODE) + public function putLocal(int $options = JSON_PRETTY_PRINT | JSON_UNESCAPED_UNICODE): bool|int { return $this->putFile(self::localPath(), $options); } - /** - * @throws \JsonException - * - * @return bool|int - */ - public function putFile(string $file, int $options = JSON_PRETTY_PRINT | JSON_UNESCAPED_UNICODE) + public function putFile(string $file, int $options = JSON_PRETTY_PRINT | JSON_UNESCAPED_UNICODE): bool|int { collect($this->toDotArray()) - ->filter(static function ($val): bool { - return ! \is_scalar($val) && null !== $val; - }) + ->filter(fn ($val) => is_scalar($val) && $val !== null) ->keys() - ->push( - 'generators.openai.parameters.prompt', - 'generators.openai.parameters.user', - 'generators.openai_chat.parameters.prompt', - 'generators.openai_chat.parameters.user', - ) - ->unique() - ->tap(function (Collection $collection): void { - $this->forget($collection->all()); - }); - - File::ensureDirectoryExists(pathinfo($file, PATHINFO_DIRNAME)); + ->tap(fn (Collection $collection) => $this->forget($collection->all())); + File::ensureDirectoryExists(dirname($file)); return File::put($file, $this->toJson($options)); } - /** - * @throws \JsonException - */ public function replaceFrom(string $file): void { $this->replace(self::readFrom($file)); @@ -159,36 +96,17 @@ public function replace(array $items): void $this->items = array_replace_recursive($this->items, $items); } - /** - * @param array|string $keys - */ public function forget($keys): void { Arr::forget($this->items, $keys); } - /** - * Convert the object into something JSON serializable. - * - * @throws \JsonException - * - * @return array - */ public function jsonSerialize(): array { return array_map(static function ($value) { - if ($value instanceof \JsonSerializable) { - return $value->jsonSerialize(); - } - - if ($value instanceof Jsonable) { - return json_decode($value->toJson(), true, 512, JSON_THROW_ON_ERROR); - } - - if ($value instanceof Arrayable) { - return $value->toArray(); - } - + if ($value instanceof \JsonSerializable) return $value->jsonSerialize(); + if ($value instanceof Jsonable) return json_decode($value->toJson(), true, 512, JSON_THROW_ON_ERROR); + if ($value instanceof Arrayable) return $value->toArray(); return $value; }, $this->all()); } @@ -200,49 +118,28 @@ public function toDotArray(): array public function toArray(): array { - return array_map(static function ($value) { - return $value instanceof Arrayable ? $value->toArray() : $value; - }, $this->all()); + return array_map(fn ($value) => $value instanceof Arrayable ? $value->toArray() : $value, $this->all()); } - /** - * {@inheritDoc} - * - * @throws \JsonException - * @noinspection JsonEncodingApiUsageInspection - * @noinspection MissingParameterTypeDeclarationInspection - */ - public function toJson($options = JSON_PRETTY_PRINT | JSON_UNESCAPED_UNICODE): string + public function toJson(int $options = JSON_PRETTY_PRINT | JSON_UNESCAPED_UNICODE): string { - return json_encode($this->jsonSerialize(), $options); + return json_encode($this->jsonSerialize(), $options, 512); } - /** - * @throws \JsonException - */ public static function readFrom(...$files): array { - $configurations = array_reduce($files, static function (array $configurations, string $file): array { - $ext = str(pathinfo($file, PATHINFO_EXTENSION)); - if ($ext->is('php')) { + $configurations = []; + foreach ($files as $file) { + $ext = pathinfo($file, PATHINFO_EXTENSION); + if ($ext === 'php') { $configurations[] = require $file; - - return $configurations; - } - - if ($ext->is('json')) { - if (! str($contents = file_get_contents($file))->jsonValidate()) { - throw InvalidJsonFileException::make($file); - } - + } elseif ($ext === 'json') { + $contents = file_get_contents($file) ?: throw InvalidJsonFileException::make($file); $configurations[] = json_decode($contents, true, 512, JSON_THROW_ON_ERROR); - - return $configurations; + } else { + throw UnsupportedConfigFileTypeException::make($file); } - - throw UnsupportedConfigFileTypeException::make($file); - }, []); - + } return array_replace_recursive(...$configurations); } }