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

Type the service registration #25359

Merged
merged 1 commit into from
Feb 10, 2021

Conversation

ChristophWurst
Copy link
Member

@ChristophWurst ChristophWurst commented Jan 28, 2021

As we add more and more registrations to the bootstrap context, these stringly-typed array become a potential source for typos and error but also a maintenance burden. To fight this I think it's worth having a few dedicated types (classes) to abstract this.

My follow-up to this will be to also abstract the service locator pattern we currently have in the services that consume registered classes. That's some infrastructure code that we should abstract away as far as possible. It's also always pretty much the same coding pattern.

Todo:

  • Finish this pattern and migrate the remaining stringly-typed registration in the context
  • Add some tests in Psalm we trust

@ChristophWurst ChristophWurst added this to the Nextcloud 22 milestone Jan 28, 2021
@ChristophWurst ChristophWurst force-pushed the enhancement/typed-service-registration branch from e933477 to b40481a Compare February 9, 2021 19:43
@ChristophWurst ChristophWurst marked this pull request as ready for review February 9, 2021 19:43
Signed-off-by: Christoph Wurst <[email protected]>
@ChristophWurst ChristophWurst force-pushed the enhancement/typed-service-registration branch from b40481a to aabd739 Compare February 10, 2021 08:44
Copy link
Member

@nickvergessen nickvergessen left a comment

Choose a reason for hiding this comment

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

🐘

@ChristophWurst
Copy link
Member Author

Psalm failure is unrelated and fixed with 130ce14

Copy link
Member

@rullzer rullzer left a comment

Choose a reason for hiding this comment

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

❤️

@ChristophWurst ChristophWurst added 4. to release Ready to be released and/or waiting for tests to finish and removed 2. developing Work in progress labels Feb 10, 2021
@ChristophWurst ChristophWurst merged commit 2f26ff4 into master Feb 10, 2021
@ChristophWurst ChristophWurst deleted the enhancement/typed-service-registration branch February 10, 2021 14:31
rullzer added a commit that referenced this pull request Feb 10, 2021
As a wise man once said:

"I like PRs that pass tests before merging"
C. Wurst, Feb 9th 2021

Signed-off-by: Roeland Jago Douma <[email protected]>
rullzer added a commit that referenced this pull request Feb 10, 2021
@ChristophWurst ChristophWurst mentioned this pull request Feb 11, 2021
5 tasks
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
4. to release Ready to be released and/or waiting for tests to finish enhancement technical debt
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants