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.

Sign up for GitHub

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

Jump to bottom

Add ctx manager tests for pep249 based db modules #450

Merged
merged 1 commit into from
Sep 1, 2023
Merged

Add ctx manager tests for pep249 based db modules #450

merged 1 commit into from
Sep 1, 2023

Conversation

GSVarsha
Copy link
Contributor

@GSVarsha GSVarsha commented Aug 31, 2023 •
edited
Loading

Add context manager tests to db modules that conform to PEP-249

  • psycopg2
  • pymysql
  • mysqlclient

@GSVarsha GSVarsha self-assigned this Aug 31, 2023
@GSVarsha GSVarsha requested review from pvital and Ferenc- August 31, 2023 07:20
@GSVarsha
Copy link
Contributor Author

Screenshot 2023-08-31 at 1 01 19 PM

pvital
pvital approved these changes Aug 31, 2023
View reviewed changes
Copy link
Member

@pvital pvital left a comment

Choose a reason for hiding this comment

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

LGTM. Great job @GSVarsha.

@GSVarsha GSVarsha mentioned this pull request Aug 31, 2023
Closed
Ferenc-
Ferenc- requested changes Aug 31, 2023
View reviewed changes
Copy link
Collaborator

@Ferenc- Ferenc- left a comment

Choose a reason for hiding this comment

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

For now, some comments on implementation details.

Ferenc-
Ferenc- requested changes Aug 31, 2023
View reviewed changes
Copy link
Collaborator

@Ferenc- Ferenc- left a comment

Choose a reason for hiding this comment

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

So far this PR is logically one change, which should be revertable and cherry-pick -able in one commit. But it is broken down to two commits. The second is correcting formalities of the first one, which carries the main value. So in the git history we would have to check multiple commits to get to one valuable change.

I suggest to put all these changes here in this case into one commit. Feel free to force push onto PR branches.
And consider make separate commits, if we deliver value separately, and or it makes sense to revert them separately if there is a need.

@instana instana deleted a comment from github-actions bot Aug 31, 2023
@GSVarsha GSVarsha force-pushed the context_manager_test branch from de9a485 to 2b0c2bc Compare August 31, 2023 09:52
@instana instana deleted a comment from github-actions bot Aug 31, 2023
@instana instana deleted a comment from github-actions bot Aug 31, 2023
Ferenc-
Ferenc- requested changes Aug 31, 2023
View reviewed changes
Copy link
Collaborator

@Ferenc- Ferenc- left a comment

Choose a reason for hiding this comment

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

So we essentially have the reformatting back again?

@GSVarsha GSVarsha force-pushed the context_manager_test branch from 2b0c2bc to b7dd293 Compare August 31, 2023 10:17
@GSVarsha
Add context manager tests to db modules that conform to PEP-249
32818ce 
Signed-off-by: Varsha GS <varsha.gs@ibm.com>

remove result

Signed-off-by: Varsha GS <varsha.gs@ibm.com>
@GSVarsha GSVarsha force-pushed the context_manager_test branch from b7dd293 to 32818ce Compare August 31, 2023 10:38
@GSVarsha
Copy link
Contributor Author

So we essentially have the reformatting back again?

Accidentally picked the wrong commit during rebase earlier. Should be good now.

@GSVarsha GSVarsha requested a review from Ferenc- August 31, 2023 10:48
@Ferenc- Ferenc- merged commit 2b95627 into master Sep 1, 2023
@Ferenc- Ferenc- deleted the context_manager_test branch September 1, 2023 07:53
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@Ferenc- Ferenc- Ferenc- approved these changes

@pvital pvital pvital approved these changes

Assignees

@GSVarsha GSVarsha

Labels
None yet
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
3 participants
@GSVarsha @pvital @Ferenc-