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.

Sign up for GitHub

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

Make get_pings include locally defined services #83

Show file tree
Hide file tree
Changes from 1 commit
Commits
Show all changes
4 commits
Select commit Hold shift + click to select a range
07abec2
Make get_pings include locally defined services
vladfedoriuk Apr 14, 2024
4192be7
Extend the CHANGELOG
vladfedoriuk Apr 14, 2024
672f967
Refactor the get_pings method
vladfedoriuk Apr 14, 2024
ea3da46
Remove an unnecessary check for type
vladfedoriuk Apr 14, 2024
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
56 changes: 42 additions & 14 deletions src/svcs/_core.py
View file
Open in desktop
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 @@
_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 @@
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 @@
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 @@
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,41 @@
Returns:
A list of services that have registered a ping callable.
"""
return [
ServicePing(
rs.name,
iscoroutinefunction(rs.ping),
rs.svc_type,
rs.ping,
self,
global_svcs = {
tp: self.registry.get_registered_service_for(tp)
for tp in self.registry
}
local_svcs = (
{
tp: self._lazy_local_registry.get_registered_service_for(tp)
for tp in self._lazy_local_registry
}
if self._lazy_local_registry is not None
else {}
)

svc_types = set(global_svcs.keys())
svc_types.update(local_svcs.keys())

pings = []
for tp in svc_types:
rs = None
if tp in local_svcs and local_svcs[tp].ping:
rs = local_svcs[tp]
elif tp in global_svcs and global_svcs[tp].ping:
rs = global_svcs[tp]
if rs is None:
continue
pings.append(
ServicePing(
rs.name,
iscoroutinefunction(rs.ping),
rs.svc_type,
rs.ping,

Check failure on line 654 in src/svcs/_core.py

View workflow job for this annotation

GitHub Actions / Pyright Codebase 

Argument of type "((...) -> Unknown) | None" cannot be assigned to parameter "_ping" of type "(...) -> Unknown" in function "__init__"
  Type "((...) -> Unknown) | None" cannot be assigned to type "(...) -> Unknown"
    Type "None" cannot be assigned to type "(...) -> Unknown" (reportArgumentType)
self,
)
)
for rs in self.registry._services.values()
if rs.ping is not None
]
return pings

def get_abstract(self, *svc_types: type) -> Any:
"""
Expand Down
72 changes: 72 additions & 0 deletions tests/test_container.py
View file
Open in desktop
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
View file
Open in desktop
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
Loading