-
Notifications
You must be signed in to change notification settings - Fork 35
Fix duplicate mech calls #233
Conversation
| ) -> tuple[str, dict[Any, Any]]: | ||
| """Parse the trades from the response.""" | ||
|
|
||
| _mech_statistics = dict(mech_statistics) |
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.
Are you just copying here? If not, is the function's type hint for mech_statistics correct?
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.
Fixed type dict to Dict globally.
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.
Not what I meant 😅
dict was fine.
So you are just copying I assume. Why are you copying?
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.
I suspect that you can avoid explicitly copying by not shadowing mech_statistics from the main scope.
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.
The method needs both the original read-only parameter (which the method doesn't care if it shadows the main one) and I also need a copy of it which I can modify. Both are required in this method: the original and the modifiable one.
| mech_data = _mech_statistics.pop(fpmmTrade["title"], {}) | ||
| statistics_table[MarketAttribute.MECH_CALLS][ | ||
| market_status | ||
| ] += mech_statistics.get(fpmmTrade["title"], {}).get("count", 0) | ||
| mech_fees = mech_statistics.get(fpmmTrade["title"], {}).get("fees", 0) | ||
| ] += mech_data.get("count", 0) | ||
| mech_fees = mech_data.get("fees", 0) |
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.
What happens if we do not pop here? The loop is for different trades afaict so what changes?
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.
If we do not pop, it will pick again the same values if there is another trade for the same question. This is an issue, because there is no relation between mech calls and trades (we don't know what mech calls are related to each trade). Hence this was the solution adopted.
In summary, this will add up the mech calls for a given question the first time it is visited. Therefore it won't distinguish which calls are related to what trade, which is not an issue, because we only want the total.
Fix duplicate mech calls
Addressess issue #232