-
-
Notifications
You must be signed in to change notification settings - Fork 19
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
base: main
Are you sure you want to change the base?
Make get_pings include locally defined services #83
Conversation
@@ -194,6 +194,10 @@ def __del__(self) -> None: | |||
stacklevel=1, | |||
) | |||
|
|||
def __iter__(self) -> Iterator[type]: |
There was a problem hiding this comment.
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
There was a problem hiding this comment.
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.
There was a problem hiding this comment.
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?
There was a problem hiding this comment.
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.
another_service_ping.assert_not_called() | ||
local_another_service_ping.assert_called_once() | ||
|
||
def test_local_services_without_pings_do_not_discard_global_pings( |
There was a problem hiding this comment.
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?
There was a problem hiding this comment.
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:
- discarding is a breaking change
- 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.
There was a problem hiding this comment.
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?
Summary
This pull request makes it possible to retrieve the pings from locally defined services, as discussed in #81 and referenced to in #82.
Pull Request Check List
main
branch – use a separate branch!docs/core-concepts.md
or one of the integration guides by hand.versionadded
,versionchanged
, ordeprecated
directives..rst
and.md
files is written using semantic newlines.