Closed AlexTheWizardL closed 2 months ago
Attention: Patch coverage is 93.33333%
with 1 line
in your changes missing coverage. Please review.
Files with missing lines | Patch % | Lines |
---|---|---|
pyledger/ledger_engine.py | 93.33% | 1 Missing :warning: |
Flag | Coverage Δ | |
---|---|---|
unittests | 58.42% <93.33%> (+0.49%) |
:arrow_up: |
Flags with carried forward coverage won't be shown. Click here to find out more.
Files with missing lines | Coverage Δ | |
---|---|---|
pyledger/ledger_engine.py | 59.42% <93.33%> (+1.04%) |
:arrow_up: |
I have changed my opinion on the best way to combine the two related methods dealing with singleton and vector arguments.
Currently, we have one method taking singleton and another method taking pd.Series as arguments:
round_to_precision(self, amount: float, currency: str) -> float
round_series_to_precision(self, amount: pd.Series, currency: pd.Series) -> pd.Series
I propose instead to implement a single method where each argument can be either a singleton or a vector. The return value is a float if both arguments are singletons and a vector of float otherwise.
round_to_precision(self, amount: float | "vector of float", currency: str | "vector of str") -> float | vector of float
Motivation: The proposed solution is more flexible, it allows to provide one singleton and one vector as argument.
Also, the method shall be open to any vector input in python, including arrays, rather than narrowly focusing on a pd.Series. This must be a quite common problem in Pyhton and we shall stick with the usual pattern used in python. We should also stick to the usual return type for such cases (array instead of pd.Series?).
This PR introduces the
round_series_to_precision()
method, which is necessary for implementing issue #35.Key points:
round_to_precision()
method, I created a new method,round_series_to_precision()
, for the vectorized version. This avoids adding type checks and splitting the logic within a single method.TestLedger()
class. These tests are expected to be refactored in milestone #2.@lasuk Please review the changes and provide feedback or approval for merging