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

refactor: disallow calling service methods directly #124

Closed
wants to merge 3 commits into from

Conversation

elonp
Copy link
Contributor

@elonp elonp commented Apr 20, 2024

If we decide that it is wrong to call methods decorated by @service and similar decorators directly, this PR would enforce that both statically and dynamically.

It does so by having the decorators add a dummy argument of type ServiceMethodArg to the methods. ServiceMethodArg is exposed publicly so downstream packages can wrap over these decorators with decorators of their own (e.g. @task and @pipeline). However, the constructor of ServiceMethodArg takes a dummy parameter of type _ServiceMethodArgArg which is private, so only the _annotations module can create a ServiceMethodArg, and therefore only that module can call the annotated methods. I think.

Not sure we actually want to go this way.

Copy link

codecov bot commented Apr 20, 2024

Codecov Report

Merging #124 (2153b23) into main (49eeaf3) will increase coverage by 0.01%.
The diff coverage is 92.68%.

Files Patch % Lines
rats-apps/src/python/rats/apps/_annotations.py 91.30% 1 Missing and 5 partials ⚠️
Additional details and impacted files
Components Coverage Δ
rats-apps ∅ <ø> (∅)
rats-devtools ∅ <ø> (∅)
rats-pipelines ∅ <ø> (∅)
rats-processors ∅ <ø> (∅)

☔ View full report in Codecov by Sentry.
📢 Have feedback on the report? Share it here.

@elonp elonp requested a review from ms-lolo April 21, 2024 00:11
) -> Callable[[C, ServiceMethodArg], T_ServiceType]: ...


def autoid_service(fn: Any) -> Any:
Copy link
Collaborator

Choose a reason for hiding this comment

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

is this functionally the same as a single function with a Union or the two types above, or is this falling back to allowing all types for fn? or is it something else?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

  1. It's not falling back to allowing all types for fn. Only calls to autoid_service that match one of the overloads will be allowed by the static analyser.
  2. I did not try Union instead of overloading in this case. But, before switching service to class, I tried the following, and it created problems with inferring T_ServiceType. Switching to class + overloading solved them
def service(
    service_id: ServiceId[T_ServiceType],
) -> Callable[[Callable[[C], T_ServiceType] | Callable[[C, ServiceType], T_ServiceType]], Callable[P, T_ServiceType]]

@@ -48,4 +49,5 @@
"group",
"method_service_id",
"service",
"ServiceMethodArg",
Copy link
Collaborator

Choose a reason for hiding this comment

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

does this have to be exposed?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

If we want to allow external packages to add decorators that interact with the service decorators, then I think it needs to be exposed.
It's needed in order to type the additional decorators I add in rats-processors (task and pipeline).

@ms-lolo
Copy link
Collaborator

ms-lolo commented Apr 22, 2024

as far as the general idea of this pr goes, i'm unsure of the value in this since it can be so easily ignored and we're using a trick to try and prevent calling the decorated function. i'm also not a fan of the decorators turning into classes, i think it makes reading them a bit more difficult.

could we sit on this for a bit and see what patterns we develop for the services layer? i don't yet have a clear view of what changes to the apis will help us scale to new use cases but i imagine we will find the rough edges over the next ~3 months while we try to use these things in more places.

@elonp elonp marked this pull request as draft April 23, 2024 15:07
@jzazo jzazo closed this Jun 6, 2024
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

3 participants