probstj / ccGains

Python package for calculating cryptocurrency trading profits and creating capital gains reports
GNU Lesser General Public License v3.0
49 stars 13 forks source link

Make relation arg optional #18

Closed bitphage closed 5 years ago

bitphage commented 5 years ago

While analyzing only base_currency:quote_currency pair, we don't need any external reference prices.

anson-vandoren commented 5 years ago

This seems like a very specific use-case that's understandable, but outside what most people are using this library for. Instead of just making it optional (and throwing errors if forgotten for the normal use case), why not subclass BagFIFO instead?

bitphage commented 5 years ago

Good point.

probstj commented 5 years ago

So maybe the best would be to not make None the default, but at least add a hint to the docstring that it is possible:

:param relation:
            A CurrencyRelation object which serves exchange rates
            between all currencies involved in trades which will later
            be added to this BagFIFO.
            If solely trades involving base_currency will be processed, 
            a CurrencyRelation object is not necessary and can be 
            `None`. In this case, if a trade between non-base 
            currencies is encountered, an exception will be raised.

Then, inside BagFIFO.pay method, raise a proper exception:

if custom_rate is not None:
    rate = Decimal(custom_rate)
elif self.relation is None:
    self._abort( 
        'Could not fetch the price for currency_pair %s_%s on '
        '%s. Please provide a CurrencyRelation object.' % (
                currency, self.currency, dtime))
else:
    try:
        rate = Decimal(
            self.relation.get_rate(dtime, currency, self.currency))
...

`

bitphage commented 5 years ago

Thanks, applied the suggestions.

probstj commented 5 years ago

thanks!