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

Reorganize abstracts & interfaces #6

Merged
merged 7 commits into from
Oct 7, 2022
Merged

Conversation

borkweb
Copy link
Member

@borkweb borkweb commented Aug 28, 2022

  • Reorganized abstract classes and interfaces into Contracts/ directories. (I like that approach from Give)
  • Switched away from StellarWP\Schema\Container in favor of using lucatume\DI52\App and lucatume\DI52\Container.

This builds on top of #5 with the intent of being part of 1.1.0.

Addresses: #8 and #9

- Reorganized abstract classes and interfaces into `Contracts/` directories.
- Switched away from `StellarWP\Schema\Container` in favor of using `lucatume\DI52\App` and `lucatume\DI52\Container`.
@borkweb borkweb requested a review from bordoni August 28, 2022 13:46
@JasonTheAdams
Copy link

@borkweb Rather than introducing a new package and container in multiple packages, I'm inclined to make it possible within package configuration for the implementing plugin to provide its own PSR-11 compatible service container instance — which is then used by the package. What do you think?

@JasonTheAdams
Copy link

Man, that PSR-11 interface sucks more than I realized. It's way too limited. It doesn't even have the ability to set items within the container, much less singletons. We may consider making our own StellarWP/Container package that's, at the very least, our own interface.

@borkweb
Copy link
Member Author

borkweb commented Sep 14, 2022

@JasonTheAdams - I agree that it'd be great to provide a way to inject a container. That being said, I'd love this library to provide a sensible default for those that do not have the desire or knowledge of how to provide a container. We definitely need to have a more lengthy conversation around this topic with some more folks.

@JasonTheAdams
Copy link

JasonTheAdams commented Sep 14, 2022

I guess that depends on the assumptions we're willing to make. If we include this as a default for every package that uses a Container, then that's increasing the size of that package (and plugin) by ~1 MB/plugin — assuming this PR gets merged and it's not 50 MB: lucatume/di52#39 😬

At this point, I'd be willing to assume the implementing plugin will provide its own Container to reduce size and complexity per package, until such a time as folks complain and make an Issue. Even then I'd still prefer to have a separate Container package we can point folks towards to implement and provide.

@borkweb borkweb marked this pull request as draft September 15, 2022 03:48
@borkweb
Copy link
Member Author

borkweb commented Sep 15, 2022

Switching this back to draft mode because #14 rethinks some of the foundational elements that these changes build on.

@borkweb borkweb added the invalid This doesn't seem right label Sep 15, 2022
@borkweb borkweb changed the base branch from feature/add-db-library to 1.1.0 October 6, 2022 19:22
@borkweb borkweb changed the title Reorganize abstracts & interfaces, adopt DI52\App. Reorganize abstracts & interfaces Oct 6, 2022
@borkweb borkweb removed the invalid This doesn't seem right label Oct 6, 2022
@borkweb borkweb marked this pull request as ready for review October 6, 2022 19:37
@borkweb
Copy link
Member Author

borkweb commented Oct 6, 2022

Alrighty, with the addition of a Config class for Container and DB objects, this PR is now solely about relocating the interfaces and abstracts.

@borkweb borkweb merged commit c0a86c8 into 1.1.0 Oct 7, 2022
@borkweb borkweb deleted the fix/relocate-contracts branch October 7, 2022 05:57
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