chore: [sc-69544] Update Marshmallow in Python lib #116
Conversation
swember
requested review from
a team,
geopet85 and
pkopac
and removed request for
a team
geopet85
left a comment
geopet85
left a comment
There was a problem hiding this comment.
@chartmogul/analytics can someone check the 3 fields?
chartmogul/api/metrics.py
Outdated
| ltv = fields.Float() | ||
| customers = fields.Integer() | ||
| asp = fields.Float() | ||
| arpa = fields.Float() |
There was a problem hiding this comment.
I have some doubt whether ltv, asp and arpa need to be float. Integer should be fine IMO, but someone from @chartmogul/analytics should check.
There was a problem hiding this comment.
Yes, you're right, asp and arpa are integers. Ltv is not, not sure why, but you can see it in the platform's spec.
| quantity = fields.Int(allow_none=True) | ||
| mrr = fields.Number(allow_none=True) | ||
| arr = fields.Number(allow_none=True) | ||
| mrr = fields.Integer(allow_none=True) |
There was a problem hiding this comment.
This comment applies to all Integer ones; we already use fields.Int, which is an alias for Integer (link). It might be better to stick to a single integer alias for consistency and homogeneity.
|
Code Climate has analyzed commit 0c8fc44 and detected 0 issues on this pull request. The test coverage on the diff in this pull request is 100.0% (50% is the threshold). This pull request will bring the total coverage in the repository to 98.5% (0.0% change). View more on Code Climate. |
swember
deleted the
dmitriydavydov/sc-69544/update-marshmallow-in-python-lib
branch
|
Hi @davidlacho, we'll release it most likely today, in a worst case tomorrow. |
4 participants
To address the issue raised here and update Marshmallow to a newer version we need to update number types mapping.