Improve GH Actions in CLI #157
Conversation
Codecov ReportAttention: Patch coverage is
Additional details and impacted files@@ Coverage Diff @@
## master #157 +/- ##
============================================
+ Coverage 72.89% 74.30% +1.40%
- Complexity 125 131 +6
============================================
Files 8 8
Lines 369 393 +24
============================================
+ Hits 269 292 +23
- Misses 100 101 +1 ☔ View full report in Codecov by Sentry. 🚀 New features to boost your workflow:
|
There was a problem hiding this comment.
- It would be handy to suggest current date as default for
Please enter the date of the extensionoption. So you just hit "Enter" and go next option. - Option [1] for
Create Github Actions workflowwould be good as default. - For
'Reusable (Recommended – uses phpBB’s maintained workflow)'language entry got this in Windows console:[1] Reusable (Recommended – uses phpBB’s maintained workflow), but not sure if it's Windows console issue with UTF8 support.
- At the final step, got this error:
In FilesystemLoader.php line 250:
Unable to find template ".github/workflows/tests.yml.twig" (looked into: [ROOT]\ext\phpbb\skeleton\skeleton).
|
It's just won't download from Github from the |
So, it can't be changed for the specific option? |
We don't want to install/set anything unless the user explicitly chooses it. So the default is always to skip or leave empty or say no. |
|
@codecov-ai-reviewer review |
|
On it! We are reviewing the PR and will provide feedback shortly. |
PR DescriptionThis pull request refactors the extension skeleton generator to improve the handling of GitHub Actions workflows and enhance code organization. The primary goal is to provide a more streamlined and user-friendly experience for developers creating new phpBB extensions, with a focus on modernizing the testing workflow. Click to see moreKey Technical ChangesKey technical changes include: 1) Introducing a choice-based selection for GitHub Actions workflows (None, Reusable, Standalone), replacing the previous boolean approach. 2) Extracting dependency checking logic into a dedicated Architecture DecisionsThe main architectural decision is to move away from a simple boolean choice for GitHub Actions to a more nuanced selection using Symfony's Dependencies and InteractionsThis pull request primarily interacts with the Symfony Console component for handling user input and generating questions. It also relies on the language files for providing user-facing text. The changes affect the generation of the Risk ConsiderationsPotential risks include: 1) Incorrectly configured GitHub Actions workflows if the choice selection logic is flawed. 2) Compatibility issues if the minimum PHP version is not aligned with the use of array spread operators (although this is not explicitly used, it was suggested in the previous review). 3) The Standalone GitHub Actions workflow is now deprecated, which might cause confusion for existing users. Thorough testing is required to ensure the new workflow selection logic functions correctly and that the generated workflows are valid. The deprecation of the Standalone workflow should be clearly communicated to users. Notable Implementation DetailsNotable implementation details include: 1) The use of a lookup table ( |
![codecov-ai[bot]](https://avatars.githubusercontent.com/in/797565?s=60&v=4){kind=small}
|
|
||
| return $return_value; | ||
| } | ||
|
|
||
| /** | ||
| * Check if all dependencies are satisfied | ||
| * | ||
| * @param array $dependencies List of dependencies to check | ||
| * @return bool | ||
| */ | ||
| private function check_dependencies(array $dependencies): bool | ||
| { | ||
| foreach ($dependencies as $depends) |
There was a problem hiding this comment.
You've extracted the dependency checking logic to a separate method check_dependencies() which is a good practice for code reusability and maintainability. However, this method returns a boolean but doesn't provide any information about which dependency failed. Consider enhancing it to return more descriptive information for better error handling or logging.
| return $return_value; | |
| } | |
| /** | |
| * Check if all dependencies are satisfied | |
| * | |
| * @param array $dependencies List of dependencies to check | |
| * @return bool | |
| */ | |
| private function check_dependencies(array $dependencies): bool | |
| { | |
| foreach ($dependencies as $depends) | |
| /** | |
| * Check if all dependencies are satisfied | |
| * | |
| * @param array $dependencies List of dependencies to check | |
| * @return array|bool Returns true if all dependencies are met, or array with failed dependency info | |
| */ | |
| private function check_dependencies(array $dependencies): bool|array | |
| { | |
| $failed_dependencies = []; | |
| foreach ($dependencies as $depends) | |
| { | |
| if (empty($this->data['components'][$depends])) | |
| { | |
| $failed_dependencies[] = $depends; | |
| } | |
| } | |
| return empty($failed_dependencies) ? true : $failed_dependencies; | |
| } |
| { | ||
| if (empty($this->data['components'][$depends])) | ||
| { | ||
| return false; | ||
| } | ||
| } | ||
| return true; | ||
| } | ||
|
|
||
| /** | ||
| * Handle GitHub Actions specific logic | ||
| */ | ||
| private function handle_github_actions(): void | ||
| { | ||
| // Lookup table of GitHub Action component settings | ||
| $github_actions_types = [ |
There was a problem hiding this comment.
The handle_github_actions() method is a good separation of concerns for handling GitHub Actions specific logic. However, the hardcoded array indices (0, 1, 2) in the lookup table might make the code less maintainable if you add more options in the future. Consider using named constants or an enum-like approach for better readability and maintainability.
| { | |
| if (empty($this->data['components'][$depends])) | |
| { | |
| return false; | |
| } | |
| } | |
| return true; | |
| } | |
| /** | |
| * Handle GitHub Actions specific logic | |
| */ | |
| private function handle_github_actions(): void | |
| { | |
| // Lookup table of GitHub Action component settings | |
| $github_actions_types = [ | |
| /** | |
| * Handle GitHub Actions specific logic | |
| */ | |
| private function handle_github_actions(): void | |
| { | |
| // Define option constants for better readability | |
| const OPTION_NONE = 0; | |
| const OPTION_REUSABLE = 1; | |
| const OPTION_STANDALONE = 2; | |
| // Lookup table of GitHub Action component settings | |
| $github_actions_types = [ | |
| self::OPTION_NONE => ['githubactions' => false, 'githubactions_custom' => false], // No (default) | |
| self::OPTION_REUSABLE => ['githubactions' => true, 'githubactions_custom' => false], // Reusable | |
| self::OPTION_STANDALONE => ['githubactions' => false, 'githubactions_custom' => true], // Standalone | |
| ]; | |
| // Rest of the method remains the same | |
| $question_text = $this->language->lang('SKELETON_QUESTION_COMPONENT_GITHUBACTIONS') . $this->language->lang('COLON'); | |
| $choices = []; | |
| foreach (array_keys($github_actions_types) as $i) | |
| { | |
| $choices[] = $this->language->lang('SKELETON_QUESTION_COMPONENT_GITHUBACTIONS_CLI', $i); | |
| } |
| $index = array_search($choice, $choices, true); | ||
|
|
||
| $component_settings = $github_actions_types[$index] ?? $github_actions_types[0]; | ||
| $this->data['components'] = array_merge( | ||
| $this->data['components'] ?? [], | ||
| $component_settings | ||
| ); | ||
| } |
There was a problem hiding this comment.
The handling of component dependencies and GitHub Actions has been improved, but I noticed that you're manually merging arrays with array_merge(). Consider using PHP 7.4+ array spread operator ([...$array1, ...$array2]) for a more modern and readable approach if your minimum PHP version supports it.
| $index = array_search($choice, $choices, true); | |
| $component_settings = $github_actions_types[$index] ?? $github_actions_types[0]; | |
| $this->data['components'] = array_merge( | |
| $this->data['components'] ?? [], | |
| $component_settings | |
| ); | |
| } | |
| $this->data['components'] = [ | |
| ...$this->data['components'] ?? [], | |
| ...$component_settings | |
| ]; |
| $components = $this->packager->get_component_dialog_values(); | ||
| foreach ($components as $component => $details) | ||
| { | ||
| foreach ($details['dependencies'] as $depends) | ||
| // Skip early as it's handled elsewhere | ||
| if ($component === 'githubactions_custom') | ||
| { | ||
| if (empty($this->data['components'][$depends])) | ||
| { | ||
| $this->data['components'][$component] = false; | ||
| continue 2; | ||
| } | ||
| continue; | ||
| } |
There was a problem hiding this comment.
In the get_component_data() method, you're now skipping the 'githubactions_custom' component early in the loop. This creates a special case that might be confusing for future developers. Consider restructuring this to handle special components in a more systematic way, perhaps through a component registry or handler pattern.
| $components = $this->packager->get_component_dialog_values(); | |
| foreach ($components as $component => $details) | |
| { | |
| foreach ($details['dependencies'] as $depends) | |
| // Skip early as it's handled elsewhere | |
| if ($component === 'githubactions_custom') | |
| { | |
| if (empty($this->data['components'][$depends])) | |
| { | |
| $this->data['components'][$component] = false; | |
| continue 2; | |
| } | |
| continue; | |
| } | |
| protected function get_component_data() | |
| { | |
| $components = $this->packager->get_component_dialog_values(); | |
| $special_components = ['githubactions', 'githubactions_custom']; | |
| foreach ($components as $component => $details) | |
| { | |
| // Handle special components with custom logic | |
| if (in_array($component, $special_components, true)) | |
| { | |
| if ($component === 'githubactions') | |
| { | |
| $this->handle_github_actions(); | |
| } | |
| continue; | |
| } | |
| // Check dependencies | |
| if (!$this->check_dependencies($details['dependencies'])) | |
| { | |
| $this->data['components'][$component] = false; | |
| continue; | |
| } | |
| // Default logic for all other components | |
| $this->data['components'][$component] = $this->get_user_input( | |
| 'component_' . $component, | |
| $details['default'] | |
| ); | |
| } | |
| } |
|
@codecov-ai-reviewer test |
|
On it! Codecov is generating unit tests for this PR. |
|
Sentry has determined that unit tests are not necessary for this PR. |
Make choosing the Github Action type of workflow a choice:
This prevents confusing and asking the user twice about creating a github action workflow