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

Feature/export command documentation #212

Conversation

olarno
Copy link
Contributor

@olarno olarno commented Nov 4, 2023

I have created a command to retrieve all available commands within the project. After obtaining the list of commands and their respective names, I have included three key elements for each command:

  • Description: A brief explanation of what the command does.
  • Synopsis: Information on the available arguments or options for the command.
  • Helper: Additional details and explanations about the command.

You can test this functionality by running the command using bin/console app:list-command. The resulting data can be found in the file tests/Command/result/defaultExport.json.

To begin testing, the objective is to:

  • Execute the command.
  • Compare the output of the command with the expected result. The two should match

Next step

  • However, I've encountered an issue with this test: the expected result provided during testing does not match the actual output.
    • I need to investigate whether additional configurations or dependencies need to be loaded during the test to ensure both the expected result and actual output are consistent.
  • I would also like to sync up with you to confirm the appropriate location for placing the code. Should it go into the framework_extra_bundle or another suitable location?

app/src/Command/ExportListCommandsCommand.php Outdated Show resolved Hide resolved
app/src/Command/ExportListCommandsCommand.php Outdated Show resolved Hide resolved
app/src/Command/ExportListCommandsCommand.php Outdated Show resolved Hide resolved
app/src/Command/ExportListCommandsCommand.php Outdated Show resolved Hide resolved
app/src/Command/ExportListCommandsCommand.php Outdated Show resolved Hide resolved
app/src/Command/ExportListCommandsCommand.php Outdated Show resolved Hide resolved
app/src/Command/ExportListCommandsCommand.php Outdated Show resolved Hide resolved
tests/Command/ExportListCommandsCommandTest.php Outdated Show resolved Hide resolved
tests/Command/ExportListCommandsCommandTest.php Outdated Show resolved Hide resolved
tests/Command/ExportListCommandsCommandTest.php Outdated Show resolved Hide resolved
@mpoiriert
Copy link
Owner

@olarno I have review the PR, here are some other notes:

For phpcs and phpstan you can do:

Make linter

If you run all the test the ConsoleIntegrationTest should fail once you more the command in the proper namespace. Just fix it.

If you run the command in the test env it's normal that you don,t have the same list of command. Just focus on what the automation test output.

Copy link
Owner

@mpoiriert mpoiriert left a comment

Choose a reason for hiding this comment

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

You need to strong type all your parameters event if they are not object:

generateBlockText(string $title, int $width = 30, string $borderChar = '#', string $paddingChar = ' '): string

Phpstorm Hightligt this.

By experience there is no better IDE than PHPStorm for Php and there is a Symfony, doctrine and other plugin that help a lot.

You should really switch to Phpstorm and get use to it.

packages/console/Command/ExportListCommandsCommand.php Outdated Show resolved Hide resolved
packages/console/Command/ExportListCommandsCommand.php Outdated Show resolved Hide resolved
packages/console/Command/ExportListCommandsCommand.php Outdated Show resolved Hide resolved
packages/console/Command/ExportListCommandsCommand.php Outdated Show resolved Hide resolved
@olarno
Copy link
Contributor Author

olarno commented Nov 9, 2023

I applied your feedback, I struggle with the test 😓

1) App\Tests\Console\Command\ExportListCommandsCommandTest::testArguments
Error: Unknown named parameter $Path

/home/wwwroot/packages/tester/Application/CommandTestTrait.php:68

@olarno olarno marked this pull request as ready for review November 9, 2023 04:20
@olarno olarno requested a review from mpoiriert November 9, 2023 04:20
@mpoiriert mpoiriert changed the base branch from master to command_documentation November 12, 2023 16:36
@mpoiriert mpoiriert merged commit a754ecf into mpoiriert:command_documentation Nov 12, 2023
29 of 30 checks passed
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