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
Uniformize tests with the new API #781
Uniformize tests with the new API #781
Changes from all commits
974c812
8f72b20
b3f98e4
f33c908
c216552
2bf4458
2f43c4d
0f83631
e42fc01
3f25493
6832a5a
65b3c73
efb6b1f
2a14e85
38f8815
12deeb6
210481b
230b238
d465c8a
3bd250e
1ff02d4
b75bb17
4c9495a
60d5af4
cc8bf4c
69a5ca4
4ea6c7d
7eabb34
87fbfea
d54b800
dfb22e9
ef32bad
c4895a9
7d82346
42589c1
0e3f615
762cd00
4e3ee5c
551ee6d
cf91154
f9b1c6e
1d844b7
b9041ff
4e6772e
a9346e6
907b586
2fa9735
8dd9467
e392610
f162bb3
b63d1d3
252fc31
e8b7a13
456cbb9
cf542c2
87893d3
8c1238d
6f83657
85c73d8
4ddca9e
d31988b
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.
Add documentation on these attributes?
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.
Why are we keeping a
count
when we havelen(self.ids)
? 🤔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.
These attributes are not introduced by this PR, and this PR has IMHO been delayed too long for opportunistic out-of-scope improvements.
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.
Good point, there is some overlap between the attributes. I think the
ids
attribute is sometimes not set. But this is also out of the scope of this PR.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 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.
Ok got it, that's to be able to mock this method in 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.
It makes sense on its own too.
self.simulation.tax_benefit_system.get_variable
is in technical parlance a train wreck. At some later point we'll want to have aget_variable()
onSimulation
, which itself delegates toTaxBenefitSystem
. Having only one call to fix will make this easier.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.
Add some documentation on these attributes?
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.
That would be needed indeed, but also to be done in another PR.