Skip to content

Conversation

@hans-thomas
Copy link
Contributor

@hans-thomas hans-thomas commented Dec 13, 2024

Hi, it adds the ability to store the generated documentation file in a configurable directory.

TODO list:

  • Local and Storage drivers should have new config, named directory with the default value documentations
  • the production_path config for Local and Storage drivers should be renamed to base_file_name with the default value documentation (without .json extension)
  • documentation file extension should be automatically extended by the .json extension
  • Local driver should check if directory exists and create it if not (on documentation saving)
  • Storage driver should create storage directory only for non Cloud drivers (on documentation saving)
  • Config version should be increased to force developers republish package config

@DenTray
Copy link
Collaborator

DenTray commented Dec 16, 2024

@hans-thomas I've updated task description, please add required changes accordingly the new task description (please feel free to open few PRs if you can divide task for small subtasks without breaking the package work)

@DenTray
Copy link
Collaborator

DenTray commented Dec 16, 2024

@hans-thomas please do not use autoclosing issues as we need to test task before closing

@hans-thomas
Copy link
Contributor Author

Hi @DenTray, would you please review this PR?

@hans-thomas hans-thomas requested a review from DenTray January 21, 2025 08:23
$this->config = config('auto-doc.drivers.local');

if (empty($this->prodFilePath)) {
$directory = str_ends_with($this->config['directory'], DIRECTORY_SEPARATOR)
Copy link
Collaborator

Choose a reason for hiding this comment

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

Suggested change
$directory = str_ends_with($this->config['directory'], DIRECTORY_SEPARATOR)
$directory = (str_ends_with($this->config['directory'], DIRECTORY_SEPARATOR))

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I made this change but I can't understand the difference.

$this->disk = Storage::disk($this->config['disk']);

if (empty($this->prodFilePath)) {
$directory = str_ends_with($this->config['directory'], DIRECTORY_SEPARATOR)
Copy link
Collaborator

Choose a reason for hiding this comment

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

as you already did the same in the local driver, let's move this logic to the BaseDriver class

Suggested change
$directory = str_ends_with($this->config['directory'], DIRECTORY_SEPARATOR)
$directory = (str_ends_with($this->config['directory'], DIRECTORY_SEPARATOR))

Force set documentation directory as empty for AutoDocControllerTest;
@hans-thomas hans-thomas force-pushed the doc-file-on-configurable-dir branch from ee90fc7 to cef046b Compare February 2, 2025 17:53
@hans-thomas hans-thomas requested a review from DenTray February 2, 2025 17:54
$this->disk = Storage::disk($this->config['disk']);

if (empty($this->prodFilePath)) {
$directory = (str_ends_with($this->config['directory'], DIRECTORY_SEPARATOR))
Copy link
Collaborator

Choose a reason for hiding this comment

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

not sure this logic required for the Storage driver as it works correctly for all next cases:

  • Storage::put('file.txt', 'content')
  • Storage::put('/path/file.txt', 'content')
  • Storage::put('path/file.txt', 'content')
  • Storage::put('/file.txt', 'content')
  • Storage::put('//file.txt', 'content')

Copy link
Contributor Author

Choose a reason for hiding this comment

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

That logic was preventing this case:

Storage::put('documentations//documentation.json', 'content')

However, it's handling this case correctly, too.

$driver = new LocalDriver();
$driver->saveTmpData(self::$tmpData);

$this->assertFileExists(self::$tmpDocumentationFilePath);
Copy link
Collaborator

Choose a reason for hiding this comment

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

Suggested change
$this->assertFileExists(self::$tmpDocumentationFilePath);
$this->assertFileExists(storage_path('documentations/documentation.json'));

Copy link
Contributor Author

Choose a reason for hiding this comment

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

These static properties are used several times in this class and I believe they are beneficial to keep tests simpler and improve the readability.

@sonarqubecloud
Copy link

sonarqubecloud bot commented Feb 8, 2025

@hans-thomas hans-thomas requested a review from DenTray February 20, 2025 06:56
@hans-thomas
Copy link
Contributor Author

Hi @DenTray, please review this PR. If everything is okay, I will cover the new lines.

@astorozhevsky astorozhevsky requested a review from Copilot August 28, 2025 09:37
Copy link

Copilot AI left a comment

Choose a reason for hiding this comment

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

Pull Request Overview

This PR introduces configurable directory support for documentation file storage, allowing users to specify where generated documentation files are saved. The changes refactor both Local and Storage drivers to support directory configuration while maintaining backward compatibility.

  • Adds a new directory configuration option with default value documentations
  • Renames production_path to base_file_name with automatic .json extension handling
  • Implements directory creation logic for the Local driver when directories don't exist

Reviewed Changes

Copilot reviewed 7 out of 7 changed files in this pull request and generated 4 comments.

Show a summary per file
File Description
config/auto-doc.php Updates configuration structure with new directory and base_file_name options, increments config version
src/Drivers/LocalDriver.php Implements directory handling logic and automatic directory creation
src/Drivers/StorageDriver.php Refactors to use new configuration structure with directory support
tests/LocalDriverTest.php Updates tests to use new configuration and adds directory creation test coverage
tests/StorageDriverTest.php Updates tests for new configuration structure and adds directory separator handling test
tests/RemoteDriverTest.php Updates temporary file path to use storage_path helper
tests/AutoDocControllerTest.php Updates tests to use new configuration options

Tip: Customize your code reviews with copilot-instructions.md. Create the file or learn how to get started.

$this->disk = Storage::disk($this->config['disk']);

if (empty($this->prodFilePath)) {
$directory = $this->config['directory'] . DIRECTORY_SEPARATOR;
Copy link

Copilot AI Aug 28, 2025

Choose a reason for hiding this comment

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

The directory separator is unconditionally added, which will create paths like 'documentations//' when the directory already ends with a separator. This should check if the directory already ends with a separator first, similar to the LocalDriver implementation.

Suggested change
$directory = $this->config['directory'] . DIRECTORY_SEPARATOR;
$directory = $this->config['directory'];
if (!str_ends_with($directory, DIRECTORY_SEPARATOR)) {
$directory .= DIRECTORY_SEPARATOR;
}

Copilot uses AI. Check for mistakes.
public function saveData(): void
{
file_put_contents($this->prodFilePath, json_encode($this->getTmpData()));
$documentationDirectory = storage_path($this->config['directory']);
Copy link

Copilot AI Aug 28, 2025

Choose a reason for hiding this comment

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

The directory path construction is inconsistent with the mainFilePath construction. When config['directory'] already ends with DIRECTORY_SEPARATOR, this will create the wrong directory path. Should use the same normalized $directory variable from lines 19-21.

Copilot uses AI. Check for mistakes.
Comment on lines +97 to +98
$documentationDirectory = 'test_directory';
if (is_dir($documentationDirectory)) {
Copy link

Copilot AI Aug 28, 2025

Choose a reason for hiding this comment

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

The test uses a hardcoded directory name that doesn't match the actual configuration being tested. This should use the same directory configuration as other tests for consistency and to properly test the actual functionality.

Suggested change
$documentationDirectory = 'test_directory';
if (is_dir($documentationDirectory)) {
$documentationDirectory = config('auto-doc.drivers.local.directory');
if (is_dir(storage_path($documentationDirectory))) {

Copilot uses AI. Check for mistakes.
{
$documentationDirectory = 'test_directory';
if (is_dir($documentationDirectory)) {
rmdir(storage_path($documentationDirectory));
Copy link

Copilot AI Aug 28, 2025

Choose a reason for hiding this comment

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

Using rmdir() without checking if the directory is empty will fail if the directory contains files. Should use a recursive directory removal or ensure the directory is empty before attempting to remove it.

Copilot uses AI. Check for mistakes.
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