Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
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
Compute partner fees #52
Compute partner fees #52
Changes from 10 commits
02899a4
91d8446
d0b566a
800d62a
f82e436
85412aa
5f89a09
26de077
a6ad730
a9d621e
24546d2
add0483
491b1c1
443c743
0542d3c
22871ec
60f4842
f56c889
8b8698e
aa89381
5e48dfc
6405f17
84ce25d
8bc2d79
3f98527
9694f41
1c7291a
f5dcae2
File filter
Filter by extension
Conversations
Jump to
There are no files selected for viewing
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This code now deviates sufficiently from the code in the circuit breaker to require actual tests.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
a
__post_init__
method could be used instead to set the remaining fields which require computation.There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Is this line required? If not, it should be removed. If yes, a small comment might help understand why it is needed.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
There is line 98 that needs to execute after the for-loop finishes, but it only makes sense if there was at least one fee policy.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Oh, i see. But to me it is simpler to write
The other two initializations are not required.
That part of the code might even benefit from a function
and then just
The small loss in efficiency should not matter compared to the increase in readability. (Though you might disagree on the readability, then feel free to ignore).
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Actually you are right and my concern was not really justified. I simplified the code and followed your first suggestion.