Skip to content

feat: ability generate withSchedule for boostrap/app.php file#43

Open
AZabolotnikov wants to merge 59 commits intomasterfrom
generate-withSchedule
Open

feat: ability generate withSchedule for boostrap/app.php file#43
AZabolotnikov wants to merge 59 commits intomasterfrom
generate-withSchedule

Conversation

@AZabolotnikov
Copy link
Copy Markdown
Contributor

Copilot AI review requested due to automatic review settings January 22, 2026 11:18
Copy link
Copy Markdown
Contributor

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 adds functionality to generate withSchedule method calls for Laravel's bootstrap/app.php file, enabling automated addition of scheduled commands. This feature complements the existing addExceptionsRender functionality in the AppBootstrapBuilder.

Changes:

  • Added new AddScheduleCommand visitor to handle schedule command insertion with environment support
  • Extended AppBootstrapBuilder with addScheduleCommand method for public API
  • Added comprehensive test coverage including edge cases for empty schedules, existing schedules, and combined operations

Reviewed changes

Copilot reviewed 9 out of 9 changed files in this pull request and generated 8 comments.

Show a summary per file
File Description
src/Visitors/AppBootstrapVisitors/AddScheduleCommand.php New visitor implementing schedule command insertion logic with support for creating withSchedule blocks when missing
src/Builders/AppBootstrapBuilder.php Added public addScheduleCommand method and Schedule facade import handling
tests/AppBootstrapBuilderTest.php Added three test cases covering empty schedule, existing schedule, and combined render scenarios
tests/fixtures/AppBootstrapBuilderTest/results/schedule.php Expected output for adding schedule commands to empty bootstrap file
tests/fixtures/AppBootstrapBuilderTest/results/schedule_exists.php Expected output for adding commands to existing withSchedule block
tests/fixtures/AppBootstrapBuilderTest/results/combine_render.php Expected output for combining exception renders and schedule commands
tests/fixtures/AppBootstrapBuilderTest/original/schedule_exists.php Original bootstrap file with empty withSchedule block for testing
src/Visitors/AppBootstrapVisitors/AbstractAppBootstrapVisitor.php Added array type hint to FORBIDDEN_NODES constant for PHP 8.3+ compatibility
src/Visitors/AddArrayPropertyItem.php Removed unused $propertyType variable from destructuring assignment

💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.

Comment thread src/Visitors/AppBootstrapVisitors/AddScheduleCommand.php Outdated
Comment thread src/Visitors/AppBootstrapVisitors/AddScheduleCommand.php Outdated
Comment thread tests/fixtures/AppBootstrapBuilderTest/results/schedule.php Outdated
Comment thread src/Visitors/AppBootstrapVisitors/AddScheduleCommand.php
Comment thread src/Builders/AppBootstrapBuilder.php
Comment thread tests/fixtures/AppBootstrapBuilderTest/results/combine_render.php Outdated
Comment thread src/Visitors/AppBootstrapVisitors/AddScheduleCommand.php Outdated
@coveralls
Copy link
Copy Markdown

coveralls commented Jan 22, 2026

Coverage Report for CI Build 24667891894

Coverage increased (+0.03%) to 97.465%

Details

  • Coverage increased (+0.03%) from the base build.
  • Patch coverage: 5 uncovered changes across 3 files (141 of 146 lines covered, 96.58%).
  • No coverage regressions found.

Uncovered Changes

File Changed Covered %
src/Traits/VisitorHelperTrait.php 56 54 96.43%
src/Visitors/AppBootstrapVisitors/AddExceptionsRender.php 12 10 83.33%
src/Visitors/AppBootstrapVisitors/AbstractAppBootstrapVisitor.php 31 30 96.77%

Coverage Regressions

No coverage regressions found.


Coverage Stats

Coverage Status
Relevant Lines: 434
Covered Lines: 423
Line Coverage: 97.47%
Coverage Strength: 9.32 hits per line

💛 - Coveralls

@AZabolotnikov AZabolotnikov self-assigned this Jan 22, 2026
@RonasIT RonasIT deleted a comment from Copilot AI Jan 23, 2026
Copy link
Copy Markdown
Contributor

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

Copilot reviewed 14 out of 14 changed files in this pull request and generated 6 comments.


💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.

Comment thread tests/AppBootstrapBuilderTest.php Outdated
Comment thread src/Traits/AstValueBuilderTrait.php Outdated
Comment thread src/Visitors/AppBootstrapVisitors/AddScheduleCommand.php Outdated
Comment thread src/Visitors/AppBootstrapVisitors/AddScheduleCommand.php Outdated
Comment thread tests/AppBootstrapBuilderTest.php Outdated
Comment thread tests/AppBootstrapBuilderTest.php Outdated
@AZabolotnikov AZabolotnikov requested a review from Copilot January 27, 2026 11:00
@artengin artengin assigned DenTray and unassigned artengin Mar 23, 2026
Copy link
Copy Markdown
Collaborator

@DenTray DenTray left a comment

Choose a reason for hiding this comment

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

@artengin let's try to make visitors as singltones

Comment thread src/Visitors/AppBootstrapVisitors/AbstractAppBootstrapVisitor.php Outdated
@DenTray DenTray assigned artengin and unassigned DenTray Mar 27, 2026
@artengin
Copy link
Copy Markdown
Contributor

@artengin let's try to make visitors as singltones

Singleton is not suitable in this case because it guarantees that only one instance of a class exists.

In our scenario, there is a need to create multiple objects (visitors) of the same class, each having its own independent state. Using a singleton would force all consumers to share the same instance, which could lead to incorrect behavior.

@artengin artengin assigned DenTray and unassigned artengin Mar 27, 2026
@artengin artengin assigned artengin and unassigned DenTray Apr 10, 2026
@artengin
Copy link
Copy Markdown
Contributor

Waiting for changes from this request: #51

@artengin
Copy link
Copy Markdown
Contributor

@codex review

Copy link
Copy Markdown

@chatgpt-codex-connector chatgpt-codex-connector Bot left a comment

Choose a reason for hiding this comment

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

💡 Codex Review

Here are some automated review suggestions for this pull request.

Reviewed commit: 96d468aba3

ℹ️ About Codex in GitHub

Codex has been enabled to automatically review pull requests in this repo. Reviews are triggered when you

  • Open a pull request for review
  • Mark a draft as ready
  • Comment "@codex review".

If Codex has suggestions, it will comment; otherwise it will react with 👍.

When you sign up for Codex through ChatGPT, Codex can also answer questions or update the PR, like "@codex address that feedback".

Comment on lines +35 to +37
$this->addImports([
'Illuminate\Support\Facades\Schedule',
]);
Copy link
Copy Markdown

Choose a reason for hiding this comment

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

P1 Badge Avoid importing facade Schedule unconditionally

addScheduleCommand() always adds Illuminate\Support\Facades\Schedule, but withSchedule() is commonly written with use Illuminate\Console\Scheduling\Schedule; for the callback parameter. Adding both imports creates a duplicate Schedule alias and the generated bootstrap/app.php will fail to parse (name is already in use) before the app can boot. This breaks valid existing bootstrap files that already follow Laravel's typed callback style.

Useful? React with 👍 / 👎.

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

The library provides removeImports() for exactly this case. If your bootstrap/app.php already has use Illuminate\Console\Scheduling\Schedule, chain removeImports() before addScheduleCommand() to drop the conflicting import first.

Comment on lines +143 to +147
$value = match ($type) {
'int' => new Int_($value),
'array' => $this->makeArrayValue($value),
'string' => new String_($value),
'float' => new Float_($value),
Copy link
Copy Markdown

Choose a reason for hiding this comment

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

P1 Badge Support callable schedule option arguments

Schedule methods such as when / skip require callable arguments, but argument conversion only handles scalars, arrays, booleans, and null. Passing a closure (e.g. new ScheduleOption('when', [fn () => true])) now throws an UnhandledMatchError during generation, so a supported scheduling method cannot be expressed safely through this API.

Useful? React with 👍 / 👎.

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

Closure-based methods like when/skip should be passed as raw code strings, they are inserted into the file as-is without AST transformation. Example:

new ScheduleOption('when', ['fn () => User::count() > 0'])

Copy link
Copy Markdown

@chatgpt-codex-connector chatgpt-codex-connector Bot left a comment

Choose a reason for hiding this comment

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

💡 Codex Review

Here are some automated review suggestions for this pull request.

Reviewed commit: 821815bb17

ℹ️ About Codex in GitHub

Codex has been enabled to automatically review pull requests in this repo. Reviews are triggered when you

  • Open a pull request for review
  • Mark a draft as ready
  • Comment "@codex review".

If Codex has suggestions, it will comment; otherwise it will react with 👍.

When you sign up for Codex through ChatGPT, Codex can also answer questions or update the PR, like "@codex address that feedback".

Comment thread src/Traits/VisitorHelperTrait.php
Comment thread src/Visitors/AppBootstrapVisitors/AbstractAppBootstrapVisitor.php
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.

5 participants