Skip to content

This issue was moved to a discussion.

You can continue the conversation there. Go to discussion →

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

Just some thoughts #7

Closed
elfjes opened this issue Jul 31, 2023 · 1 comment
Closed

Just some thoughts #7

elfjes opened this issue Jul 31, 2023 · 1 comment

Comments

@elfjes
Copy link

elfjes commented Jul 31, 2023

Hey @hynek,

I'm the guy that did a similar thing to svcs (https://github.com/elfjes/gimme-that). At EuroPython you asked for some feedback, so here we go :)

First of all, I like the simplicity of the registry/container design. Registries are for factories, and containers contain instances. Easy.

However, the register_value method seems to break this pattern. Now an instance is present at the registry-level instead of the container level. What if there'd be a Containter.add method that adds an instance (and a Container.remove to remove/forget an instance) (perhaps implement __getitem__, __setitem__ and __delitem__) Seems to me that'd make it more clean. During testing you can then also just create a new container (fixture) for every test and don't need to mess with the registry itself.

I like the ability to close/cleanup instances and the fact that it can do async cleanup. In general, having async support is nice!

Factories currently do not accept parameters. However, they can ask for other services through the 'Quality of Life' module. Would it make sense to you to allow a factory to take an (optional) container argument that can be used to get other services to reduce the
dependency on a global object. This would also be beneficial for dealing with some locality issues, such as when using threadlocal data or ContextVar. You could make it a truly optional argument by inspecting the factory function to check if it requires an argument and only then supply it.

The health check support seems nice, although it seems to me it pollutes both the registry and the container with something that is really specific. However, I currently do not have a better idea on how to implement it. There also seems to be some overlap with the on_registry_close functionality in that it allows for associating a factory with some callback. Maybe there is something there to make it cleaner?

Speaking of health checks. Would it make sense to allow ServicePing.ping to return a value? It would allow for more detailed health checks.

Finally, if you'd be open to it, we could combine our efforts and merge functionality of our two packages into one. I'm currently thinking of the following

  • you said you liked the name gimme-that. We can keep that if you want
  • I like your approach and simplicity. I do think that I added some unnecessary complexity into my design
  • I do like having a package-global registry+container+api. Like a 'Quality of Life' module, but builtin.
  • I would also like to have (optional) type hint support for defining dependencies / DI like approach. I'm thinking of an inject-factory function (name pending) that could be used like the following:
registry.register_factory(
    MyService,
    factory=inject(MyService)
)

the inject function would return a callable that looks at the signature/type hints and request those services from the container (requires a supplied Container as an argument)

@hynek
Copy link
Owner

hynek commented Aug 1, 2023

I'm the guy that did a similar thing to svcs (https://github.com/elfjes/gimme-that). At EuroPython you asked for some feedback, so here we go :)

Thanks for getting back to me with thoughtful feedback no less!

However, the register_value method seems to break this pattern. Now an instance is present at the registry-level instead of the container level. What if there'd be a Containter.add method that adds an instance (and a Container.remove to remove/forget an instance) (perhaps implement __getitem__, __setitem__ and __delitem__) Seems to me that'd make it more clean. During testing you can then also just create a new container (fixture) for every test and don't need to mess with the registry itself.

I'm not sure I follow. For me the main difference between Registries and Containers is their lifecycle. A value has the same lifetime as a Registry; so it makes sense to me to let it live in the Registry?

Factories currently do not accept parameters. However, they can ask for other services through the 'Quality of Life' module. Would it make sense to you to allow a factory to take an (optional) container argument that can be used to get other services to reduce the dependency on a global object. This would also be beneficial for dealing with some locality issues, such as when using threadlocal data or ContextVar. You could make it a truly optional argument by inspecting the factory function to check if it requires an argument and only then supply it.

That makes a lot of sense; I never quite realized that recursive resources only work with Flask's threadlocal approach, because I didn't need it elsewhere until now (full disclosure: I currently only use svcs with Flask and AIOHTTP whose integration I'm close to merging).

The health check support seems nice, although it seems to me it pollutes both the registry and the container with something that is really specific. However, I currently do not have a better idea on how to implement it.

I understand your concern, but I'm not going for purity but to get rid of common boilerplate. :)

I guess there could be a more general approach that employs some kind of metadata and filtering on that, but I think I can live with the current solution for now.

There also seems to be some overlap with the on_registry_close functionality in that it allows for associating a factory with some callback. Maybe there is something there to make it cleaner?

Does it? You mean like a general callback system on certain lifecycle stages? I mean that sounds interesting, but I suspect it's better to watch adoption/feedback to get a better idea of what's used/needed. 🤔

Speaking of health checks. Would it make sense to allow ServicePing.ping to return a value? It would allow for more detailed health checks.

It would! I'm just trying to keep the API envelope as narrow as possible for now to avoid painting myself into a corner.

Finally, if you'd be open to it, we could combine our efforts and merge functionality of our two packages into one.

That's at least currently not possible for me, because I'm iterating too fast (including the occasional git push -f) 😱. svcs as it stands is currently an internal package that I'm trying to make publicly consumable but I can't agree to anything that might slow my iteration down.

I'm currently thinking of the following

  • you said you liked the name gimme-that. We can keep that if you want

I do like it, but is it a good name for mass-adoption? 😅 I actually literally don't know; I'm not famous for great package-naming.

  • I like your approach and simplicity. I do think that I added some unnecessary complexity into my design
  • I do like having a package-global registry+container+api. Like a 'Quality of Life' module, but builtin.

I wonder what it would take to make gimme-that a convienience-wrapper around svcs?

What would you consider a good mechanic for QoL? I was thinking contextvars.

  • I would also like to have (optional) type hint support for defining dependencies / DI like approach. I'm thinking of an inject-factory function (name pending) that could be used like the following:
registry.register_factory(
    MyService,
    factory=inject(MyService)
)

the inject function would return a callable that looks at the signature/type hints and request those services from the container (requires a supplied Container as an argument)

Could you elaborate a bit more please?

Assuming MyService(x: int, y: float), you'd expect inject create a factory that does lambda container: MyService(x=container.get(int), y=container.get(float)?

Except for the container argument, would there be anything stopping you from implementing it yourself? 🤔 ← this is generally my litmus test for good APIs: build it in a way such that people can help themselves 99% of the time


Thanks again and keep the feedback coming! I'm getting increasingly excited about this project, even if it's gonna have only two users. ;)

Repository owner locked and limited conversation to collaborators Aug 1, 2023
@hynek hynek converted this issue into discussion #8 Aug 1, 2023

This issue was moved to a discussion.

You can continue the conversation there. Go to discussion →

Labels
None yet
Projects
None yet
Development

No branches or pull requests

2 participants