Skip to content

Improve logic in Balance magic methods#2764

Merged
basfroman merged 15 commits intostagingfrom
fix/roman/balance-unit
Apr 7, 2025
Merged

Improve logic in Balance magic methods#2764
basfroman merged 15 commits intostagingfrom
fix/roman/balance-unit

Conversation

@basfroman
Copy link
Collaborator

No description provided.

@basfroman basfroman requested a review from a team March 21, 2025 02:50
@basfroman basfroman self-assigned this Mar 21, 2025
@basfroman basfroman added the bug Something isn't working label Mar 21, 2025
ibraheem-abe
ibraheem-abe previously approved these changes Mar 24, 2025
Copy link
Contributor

@ibraheem-abe ibraheem-abe left a comment

Choose a reason for hiding this comment

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

LGTM overall

@basfroman basfroman added the feature new feature added label Apr 1, 2025
@basfroman basfroman requested a review from a team April 1, 2025 16:07
Copy link
Contributor

@thewhaleking thewhaleking left a comment

Choose a reason for hiding this comment

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

A bit confused here because it seems it still does the arithmetic, but just displays a warning. Is this what we want?

@basfroman
Copy link
Collaborator Author

A bit confused here because it seems it still does the arithmetic, but just displays a warning. Is this what we want?

Exactly. Right now we can't brake users' scripts. That would cause hate. We decided to display a warning. Perhaps by version 10 we'll decide to change this logic. But we have to let users know that the operation is being performed with an error if the units don't match.

@basfroman basfroman requested review from a team, ibraheem-abe and thewhaleking April 7, 2025 16:18
thewhaleking
thewhaleking previously approved these changes Apr 7, 2025
@basfroman basfroman merged commit a625991 into staging Apr 7, 2025
125 checks passed
@basfroman basfroman deleted the fix/roman/balance-unit branch April 7, 2025 16:53
@ibraheem-abe ibraheem-abe mentioned this pull request Apr 9, 2025
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

bug Something isn't working feature new feature added

Projects

None yet

Development

Successfully merging this pull request may close these issues.

3 participants