Dataclasses #3969
Dataclasses #3969
Conversation
Codecov Report
@@ Coverage Diff @@
## develop #3969 +/- ##
===========================================
+ Coverage 78.13% 78.32% +0.18%
===========================================
Files 113 117 +4
Lines 15625 14434 -1191
Branches 2182 2290 +108
===========================================
- Hits 12209 11305 -904
+ Misses 2661 2385 -276
+ Partials 755 744 -11
Continue to review full report at Codecov.
|
| @@ -111,6 +120,7 @@ def handle_channel_new(raiden: "RaidenService", event: Event): | |||
| ) | |||
| raiden.handle_and_track_state_change(new_channel) | |||
|
|
|||
| # pylint: disable=E1101 | |||
| partner_address = channel_state.partner_state.address | |||
There was a problem hiding this comment.
There was a problem hiding this comment.
So, in our case this happens when field(repr=False) is used and the field that is another dataclass. Shouldn't we just remove the field(repr=False) and have the lint working?
There was a problem hiding this comment.
Removing field(repr=False) in the case where we only disable repr for these fields is possible. However, there will be many different places where we need to override the field with something else.
There was a problem hiding this comment.
Sure, but this would fix roughly 32 instances that are just repr=True. Counted with:
ag --python 'field(repr=False)' --nobreak --nofilename | wc
There was a problem hiding this comment.
I checked this point out. Most of the disable=E1101 were added because partner_state and our_state are defined as field(repr=False). However, i would argue that setting them to be included in repr would be a bit too much because of the amount of data that will end up being logged.
Do you think we should accept this and remove field assignment to those attributes?
| @@ -605,4 +605,5 @@ def run_test_mediated_transfer_with_node_consuming_more_than_allocated_fee( | |||
| initiator_task = app0_chain_state.payment_mapping.secrethashes_to_task[secrethash] | |||
|
|
|||
| msg = "App0 should have never revealed the secret" | |||
| assert initiator_task.manager_state.initiator_transfers[secrethash].revealsecret is None | |||
| transfer_state = initiator_task.manager_state.initiator_transfers[secrethash].transfer_state | |||
| assert transfer_state != "transfer_secret_revealed" | |||
There was a problem hiding this comment.
This is not strictally equivalent to the previous test, although I don't think it is a problem, why not just use the flag?
There was a problem hiding this comment.
We both agreed that instead of using nesting the SendSecretReveal event into this object as revealsecret, it's better to either use a flag, or set the status of the transfer similar to one of your previous PRs. So i opted to go for introducing the state
| @@ -44,7 +44,7 @@ def test_v1_event_payment_sent_failed_schema(): | |||
|
|
|||
| expected = {"event": "EventPaymentSentFailed", "log_time": log_time, "reason": "whatever"} | |||
|
|
|||
| assert all(dumped.data.get(key) == value for key, value in expected.items()) | |||
There was a problem hiding this comment.
Okay, so it does look like marshmallow interface changed.
There was a problem hiding this comment.
Yes, answered previously.
| @@ -356,7 +358,9 @@ def test_deposit_must_wait_for_confirmation(): | |||
| ) | |||
| partner_model2 = partner_model1 | |||
|
|
|||
| # pylint: disable=E1101 | |||
There was a problem hiding this comment.
This is not what you want, it will disable that given warning all the way to the end of the block:
https://pylint.readthedocs.io/en/latest/user_guide/message-control.html#block-disables
That is not good. Example:
https://gist.github.com/hackaugusto/2266871f69dac442e0bd210365c38d7a
A2(A1(1)).a.a is a false positive that has to be silenced, B(1).a is a bug that should not. However because of a block annotation like this the warning about the bug is silenced.
There was a problem hiding this comment.
BTW, there are other instances of the same pattern that need to be fixed in this PR
to work with the new dataclasses
Resolves #3869, #3253
Follow-up PRs should:
frozen=Trueon dataclassespyrsistentlibrary for nested lists / dictsreprfor dataclassesTODO:
Notes to the reviewer(s)
Some changes included adding keyword arguments to function calls, because the order of which dataclasses arguments in the constructor is very specific, which can be overcome with passing kwargs.
All fields in any dataclass should NOT be
field(init=False)as this would prevent them from being serialized and included with JSON. So all fields should be included in the constructor with a default instead.Dataclasses should NOT use "quoted" types. Example:
Because marshmallow will fail to look up that string from its registry in case the class hasn't been imported yet. So we should strive for a proper code organization that doesn't cause cyclic imports and therefore, not have to use quoted types.