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

Make get_pings include locally defined services #83

Open
wants to merge 4 commits into
base: main
Choose a base branch
from
Open
Show file tree
Hide file tree
Changes from all commits
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
6 changes: 6 additions & 0 deletions CHANGELOG.md
Original file line number Diff line number Diff line change
Expand Up @@ -15,8 +15,14 @@ You can find our backwards-compatibility policy [here](https://github.com/hynek/

## [Unreleased](https://github.com/hynek/svcs/compare/24.1.0...HEAD)

### Added

- An `__iter__` method to `Registry` that yields all registered types.

### Changed

- The `get_pings` method on the `Container` now includes the locally registered services.
[#81](https://github.com/hynek/svcs/discussions/81)
- Flask: The registry is now stored on `app.extensions`, not `app.config`.
This is an implementation detail.
If you are directly accessing the registry via `app.config`, this is a breaking change, though you should ideally move to `svcs.flask.registry` anyway.
Expand Down
43 changes: 36 additions & 7 deletions src/svcs/_core.py
Original file line number Diff line number Diff line change
Expand Up @@ -24,7 +24,7 @@
isgeneratorfunction,
)
from types import TracebackType
from typing import Any, Awaitable, TypeVar, overload
from typing import Any, Awaitable, Iterator, TypeVar, overload

import attrs

Expand All @@ -46,7 +46,7 @@ def _full_name(obj: object) -> str:
_KEY_CONTAINER = "svcs_container"


@attrs.frozen
@attrs.frozen(repr=False)
class RegisteredService:
svc_type: type
factory: Callable = attrs.field(hash=False)
Expand Down Expand Up @@ -113,7 +113,7 @@ async def aping(self) -> None:
self._ping(svc)


@attrs.define
@attrs.define(repr=False)
class Registry:
"""
A central registry of recipes for creating services.
Expand Down Expand Up @@ -194,6 +194,10 @@ def __del__(self) -> None:
stacklevel=1,
)

def __iter__(self) -> Iterator[type]:
Copy link
Author

Choose a reason for hiding this comment

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

I would appreciate an input - whether adding this method is OK and if so, where / how should it get documented

Copy link
Owner

Choose a reason for hiding this comment

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

Since this looks like #66, it's worth adding, but it might better as a separate PR for the reason alone that I'm not sure how long this one will take and it seems less controversial/decision-heavy.

Copy link
Author

Choose a reason for hiding this comment

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

Could this method only be included in another PR? Or should registered services inspection be more extensive?

Copy link
Owner

Choose a reason for hiding this comment

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

Yes, adding __iter__ in a separate PR is what I meant. Two changelog entries are a strong indicator that a PR is doing too much. 🤓

I also do think that the inspection could use more knobs, but this would be a start.

"""Iterate over the registered services types."""
return iter(self._services)

def register_factory(
self,
svc_type: type,
Expand Down Expand Up @@ -454,7 +458,7 @@ def _takes_container(factory: Callable) -> bool:
T10 = TypeVar("T10")


@attrs.define
@attrs.define(repr=False)
class Container:
"""
A per-context container for instantiated services and cleanups.
Expand Down Expand Up @@ -617,17 +621,42 @@ def get_pings(self) -> list[ServicePing]:
Returns:
A list of services that have registered a ping callable.
"""
return [
pings = []
registered_local_svc_types = set()
if self._lazy_local_registry is not None:
svcs = (
self._lazy_local_registry.get_registered_service_for(tp)
for tp in self._lazy_local_registry
)
for rs in svcs:
if rs.ping is not None:
pings.append(
ServicePing(
rs.name,
iscoroutinefunction(rs.ping),
rs.svc_type,
rs.ping,
self,
)
)
registered_local_svc_types.add(rs.svc_type)
svcs = (
self.registry.get_registered_service_for(tp)
for tp in self.registry
)
pings.extend(
ServicePing(
rs.name,
iscoroutinefunction(rs.ping),
rs.svc_type,
rs.ping,
self,
)
for rs in self.registry._services.values()
for rs in svcs
if rs.ping is not None
]
and rs.svc_type not in registered_local_svc_types
)
return pings

def get_abstract(self, *svc_types: type) -> Any:
"""
Expand Down
72 changes: 72 additions & 0 deletions tests/test_container.py
Original file line number Diff line number Diff line change
Expand Up @@ -111,6 +111,7 @@ def scope():


class TestServicePing:

def test_ping(self, registry, container, close_me):
"""
Calling ping instantiates the service using its factory, appends it to
Expand All @@ -137,3 +138,74 @@ def factory():
assert close_me.is_closed
assert not container._instantiated
assert not container._on_close

def test_local_pings_are_retrieved(self, registry, container):
"""
Registering a local factory with a ping defined should make it possible to
invoke a ping for that service.
"""

def another_service_factory():
yield AnotherService()

another_service_ping = Mock(spec_set=["__call__"])
container.register_local_factory(
AnotherService, another_service_factory, ping=another_service_ping
)

(svc_ping,) = container.get_pings()
svc_ping.ping()

another_service_ping.assert_called_once()

def test_local_pings_override_global_pings(self, registry, container):
"""
If a local factory overwrites an existing, global one, and the local factory has a ping
defined, the local ping should be used.
"""

def another_service_factory():
yield AnotherService()

another_service_ping = Mock(spec_set=["__call__"])
local_another_service_ping = Mock(spec_set=["__call__"])
registry.register_factory(
AnotherService, another_service_factory, ping=another_service_ping
)
container.register_local_factory(
AnotherService,
another_service_factory,
ping=local_another_service_ping,
)

(svc_ping,) = container.get_pings()
svc_ping.ping()

another_service_ping.assert_not_called()
local_another_service_ping.assert_called_once()

def test_local_services_without_pings_do_not_discard_global_pings(
Copy link
Author

Choose a reason for hiding this comment

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

I assumed such behavior would be the most "predictable" from the end-user perspective, but I was not quite sure about this change. Would you find it valid to make it work like that?

Copy link
Owner

Choose a reason for hiding this comment

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

yeah hm this is hard. There's two contradicting issues here:

  1. discarding is a breaking change
  2. If I overwrite a definition, that's what I want to use.

But more specifically, it's the decision of the user to add a new definition of a service with a ping. I feel like discarding follows the wish of the user? They can define a new type based on the old type if they want to preserve the old definition.

Given that svcs core concept are types, I feel like we should respect it when a user says "Connection is now this new thing."

But don't change anything, yet.

Copy link
Author

Choose a reason for hiding this comment

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

Ok, so regardless of whether a new definition contains a ping, the latest defined service (could be local) must be used?

self, registry, container
):
"""
If a local factory overwrites an existing, global one, but the local factory does not have a ping
defined, the global ping should be used.
"""

def another_service_factory():
yield AnotherService()

another_service_ping = Mock(spec_set=["__call__"])
registry.register_factory(
AnotherService, another_service_factory, ping=another_service_ping
)
container.register_local_factory(
AnotherService,
another_service_factory,
ping=None,
)

(svc_ping,) = container.get_pings()
svc_ping.ping()

another_service_ping.assert_called_once()
18 changes: 17 additions & 1 deletion tests/test_registry.py
Original file line number Diff line number Diff line change
Expand Up @@ -17,7 +17,7 @@

from .fake_factories import async_str_gen_factory, str_gen_factory
from .helpers import nop
from .ifaces import AnotherService, Interface, Service
from .ifaces import AnotherService, Interface, Service, YetAnotherService


needs_working_async_mock = pytest.mark.skipif(
Expand Down Expand Up @@ -50,6 +50,12 @@ def test_repr_empty(self, registry):
"""
assert "<svcs.Registry(num_services=0)>" == repr(registry)

def test_iter_empty(self, registry):
"""
Iterating over an empty registry returns nothing.
"""
assert not list(registry)

def test_repr_counts(self, registry):
"""
repr counts 2 registered services as 2.
Expand Down Expand Up @@ -281,6 +287,16 @@ def test_contains(self, registry):
assert Service in registry
assert AnotherService not in registry

def test_iter(self, registry):
"""
Iterating over a registry returns all registered services types.
"""
registry.register_factory(Service, Service)
registry.register_factory(AnotherService, AnotherService)
registry.register_value(YetAnotherService, YetAnotherService)

assert {Service, AnotherService, YetAnotherService} == set(registry)

def test_gc_warning(self, recwarn):
"""
If a registry is gc'ed with pending cleanups, a warning is raised.
Expand Down