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

Remove lends and loans from the store on zeroing out #61

Merged
merged 11 commits into from
Aug 23, 2023

Conversation

MikeHathaway
Copy link
Contributor

@MikeHathaway MikeHathaway commented Aug 22, 2023

  • Remove Lend and Loan entities from the store when they zero out their attributes.
  • Add new _handleBucketBankruptcy function

@MikeHathaway MikeHathaway requested a review from EdNoepel August 22, 2023 19:07
@MikeHathaway MikeHathaway marked this pull request as draft August 22, 2023 19:54
@MikeHathaway MikeHathaway marked this pull request as ready for review August 22, 2023 21:51
Copy link
Contributor

@EdNoepel EdNoepel left a comment

Choose a reason for hiding this comment

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

I'm not excited about removeLendFromStore and removeLoanFromStore, especially the naming. I was tempted to recommend they be included in updateBucketLends and updateAccountLoans, but I see why that coupling would be undesirable.

How about we rework these methods as saveOrRemoveLend and saveOrRemoveLoan? The caller calls this method in lieu of entity.save, the method handles the branch, and no return value is needed.

@MikeHathaway
Copy link
Contributor Author

I'm not excited about removeLendFromStore and removeLoanFromStore, especially the naming. I was tempted to recommend they be included in updateBucketLends and updateAccountLoans, but I see why that coupling would be undesirable.

How about we rework these methods as saveOrRemoveLend and saveOrRemoveLoan? The caller calls this method in lieu of entity.save, the method handles the branch, and no return value is needed.

Updated as recommended in a4ad178

I had originally just done the removal in those methods which worked with minimal changes but then I became concerned about the maintainability of that approach especially with the dependency on method ordering for updateBucketLends and updateAccountLends

@MikeHathaway MikeHathaway requested a review from EdNoepel August 23, 2023 16:11
Copy link
Contributor

@EdNoepel EdNoepel left a comment

Choose a reason for hiding this comment

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

Much thanks; LGTM.

@MikeHathaway MikeHathaway merged commit b6a6191 into develop Aug 23, 2023
@MikeHathaway MikeHathaway deleted the splice-zero-entities branch August 23, 2023 19:27
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