-
Notifications
You must be signed in to change notification settings - Fork 89
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
Count Workflow Executions #203
base: master
Are you sure you want to change the base?
Count Workflow Executions #203
Conversation
lib/temporal.rb
Outdated
@@ -29,7 +29,8 @@ module Temporal | |||
:fail_activity, | |||
:list_open_workflow_executions, | |||
:list_closed_workflow_executions, | |||
:query_workflow_executions | |||
:query_workflow_executions, | |||
count_workflow_executions |
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.
This looks like a syntax error 👁️
lib/temporal/client.rb
Outdated
@@ -409,6 +409,10 @@ def query_workflow_executions(namespace, query, next_page_token: nil, max_page_s | |||
Temporal::Workflow::Executions.new(connection: connection, status: :all, request_options: { namespace: namespace, query: query, next_page_token: next_page_token, max_page_size: max_page_size }.merge(filter)) | |||
end | |||
|
|||
def count_workflow_executions(namespace, query) |
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 don't think that other SDKs actually have this method exposed. With that in mind, let's not add it to the Client
, but instead only expose it on the connection itself and possibly make the connection public. This way you can access the low-level API if needed, but we're not committing to supporting it via the high-level classes
namespace: namespace, | ||
query: query | ||
) | ||
client.count_workflow_executions(request) |
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.
wait, this is also missing an end
. Something is not right with this PR
Summary
This PR fixes implements the count_workflow_executions function and adds an integration test to check this works!
Test Plan