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

General readability/extendability improvements #157

Merged
merged 6 commits into from
Jan 20, 2025

Conversation

stuart-elliott
Copy link
Contributor

This is a followup from pull request #145 which includes the most recent changes from master.

- Splitting the Package class into traits to improve readability
- Splitting the PackageServiceProvider into individual methods
- All test back to green
- Removed duplicate config_path line from the boot configs method in the service provider

- Merged the ?string checks into the hasViews & hasInertiaComponents from the previous pull requests.

- Composer test reporting green
- Rebasing against master to pull in the recent updates
@Sophist-UK
Copy link

I fully support this PR (as I did the previous one) - this is a good modularisation of the code and makes it easier for people to add further functionality - I have a some ideas I am working on to add e.g. Livewire support and support a range of other ServiceProvider features, but it makes sense for me to base my PR on this one so I don't want to submit it until this one has been merged.

Copy link

@Sophist-UK Sophist-UK left a comment

Choose a reason for hiding this comment

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

  1. I have no idea why you keep submitting new PRs rather than simply fixing the old ones. PLEASE STOP DOING THIS, as all the history of people like me supporting your PR gets lost. I have no idea why you resubmitted this because it seems to have the same code as Rebased PR #145 from Main #152, and my comments to Rebased PR #145 from Main #152 have NOT been dealt with.
  2. I have no idea why you force pushed to General readability/extendability improvements #145 or Rebased PR #145 from Main #152 to clear out the changes but PLEASE STOP DOING THIS. If you insist on opening a new replacement then just close the old one.

Here are my comments from #152:
In general I like the modular concepts put forward here, with the following comments:

  1. I like the use of modular Traits in Package.php; and
  2. I like the splitting of the code in the PackageServiceProvider.php; however
  3. I would like to see the PackageServiceProvider.php similarly modularised into Traits (named processX to match the hasX trait); and
  4. I see no reason why GenerateMigrationName should be an Action (which are supposed to apply to users) rather than simply being a utility method called by other code.
  5. I would like to see the Test files themselves moved into matching subdirectories (moving them should all that is needed as they should still run).

@freekmurze
Copy link
Member

Thanks for your work on this.

I'll merge this in and slightly polish it.
@Sophist-UK Feel free to submit PRs for the improvements you'd like to see.

@freekmurze freekmurze merged commit 390493e into spatie:main Jan 20, 2025
25 checks passed
@Sophist-UK
Copy link

Sophist-UK commented Jan 20, 2025

Great - as a heads up I want to add functionality for Livewire (to match Inertia), to fully utilise all the capabilities of the ServiceProvider object, to allow user to override the default directories for every hasX function and to improve the coverage of some tests which are actually masking some issues.

This is already partly written, so you might want to wait before issuing a formal version upgrade.

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.

3 participants