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

feat: Add RPC request builder class for additional filters #372

Merged
merged 3 commits into from
Feb 27, 2024

Conversation

silentworks
Copy link
Contributor

@silentworks silentworks commented Feb 24, 2024

What kind of change does this PR introduce?

Add additional filters to rpc calls to match that of the JavaScript library.

What is the current behavior?

You currently cannot use limit, range, select and some other filters with a .rpc call.

What is the new behavior?

You can now use limit, range, select and some other filters with a .rpc call.

Additional context

The other related PR in the supabase-py repo should be merged first supabase/supabase-py#702

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: Enhancement

PR Summary: This pull request introduces a new BaseRPCRequestBuilder class and integrates it with the existing async and sync request builder classes. The primary goal is to extend the functionality of RPC calls, allowing them to utilize additional filters such as 'limit', 'range', 'select', among others. This enhancement aligns the Python library's capabilities more closely with those of its JavaScript counterpart, addressing a gap in the current behavior where such filters were not applicable to '.rpc' calls.

Decision: Comment

📝 Type: 'Enhancement' - 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:

  • Ensure comprehensive testing of the new filters in various scenarios to guarantee they behave as expected across both synchronous and asynchronous contexts.
  • Consider adding more detailed documentation or examples in the docstrings or the project's official documentation to help users understand how to use the new filtering capabilities with RPC calls.
  • Review the implementation of unconventional methods, such as the use of get_origin_and_cast for superclass initialization in BaseRPCRequestBuilder, to ensure they are the best approach for maintainability and readability.
postgrest/base_request_builder.py: The new code introduces additional complexity and could benefit from simplification to improve maintainability.

Thank you for the effort put into these changes. I've noticed that the new code introduces additional complexity, particularly with the introduction of the BaseRPCRequestBuilder class and its methods. Here are a few points of concern:

  1. The new class and methods significantly increase the codebase's length, making it more challenging to maintain.
  2. The initialization of BaseRPCRequestBuilder involves complex inheritance and typing patterns that could be simplified for better readability and maintainability.
  3. Direct manipulation of HTTP headers within methods adds another layer of complexity, requiring developers to have a deeper understanding of the implications of these changes.

To reduce complexity and improve maintainability, consider the following suggestions:

  • Refactor common functionality between the new and existing classes to reduce code duplication and leverage shared behavior.
  • Simplify the initialization process in BaseRPCRequestBuilder to make it more straightforward and readable.
  • Ensure that the purpose and usage of the new methods are clear, possibly by adding more documentation or simplifying their interfaces.

Here's a simplified example that demonstrates a potential approach to reducing complexity:

class BaseRequestBuilder:
    def __init__(self, session, headers, params):
        self.session = session
        self.headers = headers
        self.params = params

class BaseRPCRequestBuilder(BaseRequestBuilder):
    def __init__(self, session, headers, params):
        super().__init__(session, headers, params)

    def select(self, *columns):
        self.params = self.params.add("select", ",".join(columns))
        self.headers["Prefer"] = "return=representation"
        return self

This approach aims to maintain the functionality you've introduced while making the codebase more approachable and easier to maintain.

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.

Copy link

codecov bot commented Feb 24, 2024

Codecov Report

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

Project coverage is 94.79%. Comparing base (0faa8c3) to head (19c5b2b).

❗ Current head 19c5b2b differs from pull request most recent head ac05422. Consider uploading reports for the commit ac05422 to get more accurate results

Files Patch % Lines
postgrest/base_request_builder.py 35.71% 9 Missing ⚠️
Additional details and impacted files
@@            Coverage Diff             @@
##           master     #372      +/-   ##
==========================================
- Coverage   95.31%   94.79%   -0.53%     
==========================================
  Files          28       28              
  Lines        1580     1594      +14     
==========================================
+ Hits         1506     1511       +5     
- Misses         74       83       +9     

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

@silentworks silentworks changed the title Add RPC request builder class for additional filters feat: Add RPC request builder class for additional filters Feb 24, 2024
anand2312
anand2312 previously approved these changes Feb 25, 2024
@silentworks silentworks merged commit 0002e8f into master Feb 27, 2024
6 checks passed
@silentworks silentworks deleted the silentworks/rpc-request-builder branch February 27, 2024 15: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.

3 participants