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

Rebased PR #145 from Main #152

Closed
wants to merge 0 commits into from
Closed

Conversation

stuart-elliott
Copy link
Contributor

  • This should hopefully do the trick :)

@Sophist-UK
Copy link

Description from #145:

  • Splitting the Package class into traits to improve readability
  • Splitting the PackageServiceProvider into individual methods
  • All test back to green
  • Hope this helps :)

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.

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; and
  4. 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).

I have already added the description from #145, however the PR title should be updated to describe the PR (i.e. by reusing the title from the previous PR) and not describe the historical progression.

TL;DR: (For what it is worth) I support the merging of this PR.

I have been working on a PR of my own to:

  • Add a few additional tests
  • Modularise the code
  • Clean up the code
  • Add additional modules for:
    • Livewire views (to match the Inertia support that already exists)
    • Other things that can be loaded in a Service Provider.

but I will wait and see whether this PR is merged before working on my PR further.

@freekmurze
Copy link
Member

Could you rebase against main to fix the conflicts?

@stuart-elliott
Copy link
Contributor Author

  • I like the use of modular Traits in Package.php; and
  • I like the splitting of the code in the PackageServiceProvider.php; however
  • I would like to see the PackageServiceProvider.php similarly modularised into Traits; and

To be honest, I actually already did a part 2 for this pull request that does exactly what you mentioned, but never bothered submitting it as presumed nobody was interested. Screenshots below.

image image image

@stuart-elliott
Copy link
Contributor Author

Could you rebase against main to fix the conflicts?

Done new pull request #157 submitted with the latest updates from master.
All tests reporting green.

@Sophist-UK
Copy link

Please STOP force-pushing to Github. It makes any reviews much much more difficult.

In terms of the screen shots of code I don't like the names of the additional traits - in this context register and boot have specific meanings. Personally I think the traits should be named processX to match with hasX.

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