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

Update explain functionality to show results #371

Merged
merged 13 commits into from
Feb 29, 2024

Conversation

silentworks
Copy link
Contributor

@silentworks silentworks commented Feb 23, 2024

What kind of change does this PR introduce?

WIP: do not merge as yet

Update the explain functionality to show the result instead of just not working

What is the current behavior?

This doesn't work with sane defaults when called without setting the format, to match that of the JS library the default format should be text and this requires the return type to change to accommodate this.

What is the new behavior?

Feel free to include screenshots if it includes visual changes.

Additional context

Add any other context or screenshots.

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 enhancements to the explain functionality within the PostgREST request builders. It aims to improve the way results are displayed by refining the response handling logic and updating the format parameter to use type-safe literals. The changes span across both asynchronous and synchronous request builder modules, as well as the base request builder, indicating a comprehensive update to the functionality.

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:

  • Given the scope and impact of the changes, it would be beneficial to ensure thorough testing across various scenarios to validate the new behavior. This includes testing with different formats and ensuring backward compatibility where applicable.
  • Consider adding more detailed documentation or comments around the newly introduced text_from_http_request_response method and the use of Literal for the format parameter. This will help maintainers and users of the library understand the rationale behind these changes and how to leverage the new functionality effectively.
  • Review the PR description to ensure it fully captures the extent of the changes made. Including examples or more detailed explanations of the new behavior could enhance understanding for reviewers and future contributors.
postgrest/base_request_builder.py: The proposed changes enhance code clarity, type safety, and functionality without adding unnecessary complexity.

After reviewing the proposed changes, I believe they do not introduce unnecessary complexity to the codebase. Instead, these modifications enhance the code's functionality, readability, and maintainability:

  1. The addition of Literal for type hinting, specifically in the explain method, makes the expected argument values clearer and prevents potential bugs by restricting the format parameter to either "text" or "json". This is a good practice that enhances the code's robustness.

  2. The new text_from_http_request_response method addresses a specific case, extending the class's functionality in a straightforward manner. It follows existing patterns in the code, making it easy to understand and maintain.

  3. The changes made to string formatting and line breaks improve readability without adding logical complexity. These stylistic adjustments make the code easier to follow and review.

Overall, these changes are aligned with modern Python practices, focusing on type safety and code clarity. They are thoughtful improvements that should make the codebase more robust and easier to work with.

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.

postgrest/_async/request_builder.py Show resolved Hide resolved
postgrest/_sync/request_builder.py Show resolved Hide resolved
Copy link

codecov bot commented Feb 23, 2024

Codecov Report

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

Project coverage is 95.79%. Comparing base (b858685) to head (8f8322d).
Report is 1 commits behind head on master.

❗ Current head 8f8322d differs from pull request most recent head b5e7236. Consider uploading reports for the commit b5e7236 to get more accurate results

Files Patch % Lines
postgrest/_async/request_builder.py 87.50% 1 Missing ⚠️
postgrest/_sync/request_builder.py 87.50% 1 Missing ⚠️
Additional details and impacted files
@@            Coverage Diff             @@
##           master     #371      +/-   ##
==========================================
+ Coverage   95.72%   95.79%   +0.06%     
==========================================
  Files          28       28              
  Lines        1639     1666      +27     
==========================================
+ Hits         1569     1596      +27     
  Misses         70       70              

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

mansueli
mansueli previously approved these changes Feb 23, 2024
@mansueli
Copy link
Member

We might need to add some tests before merging

@silentworks
Copy link
Contributor Author

@mansueli I think these tests still apply here https://github.com/supabase-community/postgrest-py/blob/master/tests/_async/test_request_builder.py#L138-L152 as only the return type change based on the plan.

@todo I wasn't able to get local postgREST responding to API commands. Using a supabase instance for now
mansueli
mansueli previously approved these changes Feb 27, 2024
tests/_sync/client.py Outdated Show resolved Hide resolved
@silentworks silentworks merged commit 3e0ea2e into master Feb 29, 2024
7 checks passed
@silentworks silentworks deleted the silentworks/explain-functionality branch February 29, 2024 01:05
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