Skip to content

Conversation

orhankislal
Copy link
Contributor

@orhankislal orhankislal commented Oct 1, 2025

Description

Add Async class for Azure PostgreSQL integration

Type of Change

Please delete options that are not relevant.

  • Bug fix (non-breaking change which fixes an issue)
  • New feature (non-breaking change which adds functionality)
  • Breaking change (fix or feature that would cause existing functionality to not work as expected)
  • This change requires a documentation update

How Has This Been Tested?

Your pull-request will likely not be merged unless it is covered by some form of impactful unit testing.

  • I added new unit tests to cover this change
  • I believe this change is already covered by existing unit tests

Suggested Checklist:

  • I have performed a self-review of my own code
  • I have commented my code, particularly in hard-to-understand areas
  • I have made corresponding changes to the documentation
  • I have added Google Colab support for the newly added notebooks.
  • My changes generate no new warnings
  • I have added tests that prove my fix is effective or that my feature works
  • New and existing unit tests pass locally with my changes
  • I ran uv run make format; uv run make lint to appease the lint gods

@dosubot dosubot bot added the size:XXL This PR changes 1000+ lines, ignoring generated files. label Oct 1, 2025
@orhankislal
Copy link
Contributor Author

The test claims a low coverage percentage but with an actual database present, the rate is at about 79%.
Screenshot 2025-10-01 210459

Copy link
Collaborator

@logan-markewich logan-markewich left a comment

Choose a reason for hiding this comment

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

Why does this have to be a completely different class? Why not just implement the async methods in the existing class? (No other class takes this approach in the repo, feels kind of weird)

@orhankislal
Copy link
Contributor Author

The initial idea was to keep the common code in a different package so that all of the Azure PostgreSQL integrations can share them. At some point we will refactor them once they are available to be imported. We use psycopg_pool which has separate pools for sync and async connections. We can try to accept both as an argument but that would make the code itself very ugly with requirements to if check all over the place. I don't think using both sync and async operations within a single class is that common of a use case. Please let me know if that is a hard requirement.

@logan-markewich
Copy link
Collaborator

logan-markewich commented Oct 6, 2025

Not a hard requirement, but it is pretty awkward and not aligned with any other integration. Likely a friction point

Other integrations handle sync/async but just having a get_client() or get_aclient() function to get the corresponding client (or raise an error if missing). This avoids the issue of "having checks everywhere" that you mentioned.

Users can't always control if a sync or async endpoint is being used

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
size:XXL This PR changes 1000+ lines, ignoring generated files.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants