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

liquidation-crud-test #627

Merged
merged 3 commits into from
Feb 24, 2025
Merged

Conversation

shamoo53
Copy link
Contributor

crud test completed

@djeck1432 djeck1432 self-requested a review February 24, 2025 07:17
Copy link
Owner

@djeck1432 djeck1432 left a comment

Choose a reason for hiding this comment

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

Fix pylint issue

@djeck1432 djeck1432 linked an issue Feb 24, 2025 that may be closed by this pull request
@djeck1432
Copy link
Owner

Please, sync with main branch @shamoo53

@djeck1432
Copy link
Owner

@shamoo53 fix please also pylint issue

@shamoo53
Copy link
Contributor Author

I will do that now

@shamoo53
Copy link
Contributor Author

Hi @djeck1432 fixed pylint issue

Copy link
Owner

@djeck1432 djeck1432 left a comment

Choose a reason for hiding this comment

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

Please, do not remove my comments and answer on it

def commit(self):
pass

def query(self, model):
Copy link
Owner

Choose a reason for hiding this comment

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

why do you need it? if you don't call it?

@shamoo53
Copy link
Contributor Author

The commit method is required to simulate the transaction interface. Even though it performs no operation in tests, it allows our code to call commit() without errors, ensuring that our test environment mirrors the production interface where commit would actually execute a transaction commit.

Copy link
Owner

@djeck1432 djeck1432 left a comment

Choose a reason for hiding this comment

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

LGTM

@djeck1432 djeck1432 merged commit a6df6d8 into djeck1432:main Feb 24, 2025
4 checks passed
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.

[Margin App] Add test case for liquidation CRUD
3 participants