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

Document and clean up subscriptions client limit behaviour #1071

Merged
merged 1 commit into from
Oct 25, 2024

Conversation

tbarsballe
Copy link
Contributor

@tbarsballe tbarsballe commented Oct 24, 2024

Related Issue(s):

Closes #1069
Closes #1070

Proposed Changes:

For inclusion in changelog (if applicable):

  1. Removed the unused limit parameter of SubscriptionsClient.get_results_csv

Not intended for changelog:

  1. Added documentation of the limit=0 behaviour to other SubscriptionsClient methods where this is still used(list_subscriptions, get_results)

PR Checklist:

  • This PR is as small and focused as possible
  • If this PR includes proposed changes for inclusion in the changelog, the title of this PR summarizes those changes and is ready for inclusion in the Changelog.
  • I have updated docstrings for function changes and docs in the 'docs' folder for user interface / behavior changes
  • This PR does not break any examples or I have updated them

@@ -310,8 +310,7 @@ async def get_results_csv(
"queued",
"processing",
"failed",
"success"]]] = None,
limit: int = 100,
Copy link
Contributor

Choose a reason for hiding this comment

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

We had a Slack discussion and found that this arg is unused, undocumented and was never required. This theoretically could be a breaking change for somebody, but removing it seems reasonable

@asonnenschein
Copy link
Contributor

Confirming after reviewing this PR and Subscription API's /results endpoint that limit is undocumented and unused, and I agree that this should be removed.

@tbarsballe tbarsballe merged commit 4f9693c into main Oct 25, 2024
10 checks passed
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.

get_results_csv limit parameter has no effect Limit behaviour not documented in the subscriptions API
3 participants