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

fix: Update rpc return type #702

Merged
merged 2 commits into from
Feb 26, 2024
Merged

Conversation

silentworks
Copy link
Contributor

@silentworks silentworks commented Feb 24, 2024

What kind of change does this PR introduce?

Bug fix

This PR should be merged before supabase/postgrest-py#372 in order for the version bump to happen correctly.

What is the current behavior?

Returning wrong return type from .rpc method, also there is no default for params which means you have to pass an empty dict even if the function doesn't have any parameters.

What is the new behavior?

Returning correct return type from .rpc method and now has None as default for params so you don't need to pass a dict if there are no parameters to the function.

Additional context

In relation to #700 and supabase/postgrest-py#372

Copy link
Contributor

@sourcery-ai sourcery-ai bot left a comment

Choose a reason for hiding this comment

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

PR Type: Bug fix

PR Summary: The pull request updates the .rpc method in both asynchronous and synchronous client modules of a Supabase client library to return the correct type. It introduces a default parameter for the params argument and updates the return type to a more specific AsyncRPCFilterRequestBuilder and SyncRPCFilterRequestBuilder for the asynchronous and synchronous versions, respectively.

Decision: Comment

📝 Type: 'Bug fix' - not supported yet.
  • Sourcery currently only approves 'Typo fix' PRs.
✅ Issue addressed: this change correctly addresses the issue or implements the desired feature.
No details provided.
✅ Small diff: the diff is small enough to approve with confidence.
No details provided.

General suggestions:

  • While the PR successfully addresses the issue of returning the correct type from the .rpc method, it introduces a high-risk bug by using a mutable default argument ({}). It's recommended to use None as a default value and initialize the dictionary within the method if params is None. This change will prevent potential bugs related to the mutable default argument being shared across method calls.
  • Ensure that the documentation and type annotations are updated accordingly to reflect the change in the default value of the params parameter if the suggested fix is applied.
  • Consider reviewing all methods with default parameters to ensure that mutable default arguments are not being used elsewhere in the codebase, as this could lead to similar issues.

Thanks for using Sourcery. We offer it for free for open source projects and would be very grateful if you could help us grow. If you like it, would you consider sharing Sourcery on your favourite social media? ✨

Share Sourcery

Help me be more useful! Please click 👍 or 👎 on each comment to tell me if it was helpful.

supabase/_async/client.py Outdated Show resolved Hide resolved
supabase/_async/client.py Outdated Show resolved Hide resolved
supabase/_sync/client.py Outdated Show resolved Hide resolved
@silentworks silentworks changed the title Update rpc return type fix: Update rpc return type Feb 24, 2024
Copy link

codecov bot commented Feb 24, 2024

Codecov Report

Attention: Patch coverage is 33.33333% with 6 lines in your changes are missing coverage. Please review.

Project coverage is 43.97%. Comparing base (9357140) to head (2f97e6b).

Files Patch % Lines
supabase/_async/client.py 0.00% 4 Missing ⚠️
supabase/_sync/client.py 60.00% 2 Missing ⚠️
Additional details and impacted files
@@            Coverage Diff             @@
##             main     #702      +/-   ##
==========================================
- Coverage   44.51%   43.97%   -0.54%     
==========================================
  Files          13       13              
  Lines         328      332       +4     
==========================================
  Hits          146      146              
- Misses        182      186       +4     

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

@silentworks silentworks merged commit 4130d20 into main Feb 26, 2024
5 of 7 checks passed
@silentworks silentworks deleted the silentworks/update-rpc-return-type branch February 26, 2024 11:17
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.

2 participants