New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

MAINT: Add better repr for transactions #1746

Merged
merged 1 commit into from May 8, 2017

Conversation

Projects
None yet
4 participants
@freddiev4
Contributor

freddiev4 commented Apr 10, 2017

The repr for a Transaction object now looks like:

Transaction(order_id=20, sid=Asset(0), dt=2015-01-01 00:00:00, amount=10, price=50)

@freddiev4 freddiev4 force-pushed the update-transaction-repr branch from 7b35441 to c983c23 Apr 10, 2017

@jbredeche

This comment has been minimized.

Member

jbredeche commented Apr 10, 2017

Nice! Can you add price too?

@coveralls

This comment has been minimized.

coveralls commented Apr 10, 2017

Coverage Status

Coverage decreased (-0.005%) to 87.521% when pulling c983c23 on update-transaction-repr into 4b861fb on master.

@coveralls

This comment has been minimized.

coveralls commented Apr 10, 2017

Coverage Status

Coverage decreased (-0.005%) to 87.521% when pulling c983c23 on update-transaction-repr into 4b861fb on master.

@freddiev4 freddiev4 force-pushed the update-transaction-repr branch from c983c23 to 910bed0 Apr 10, 2017

@coveralls

This comment has been minimized.

coveralls commented Apr 10, 2017

Coverage Status

Coverage decreased (-0.005%) to 87.526% when pulling 910bed0 on update-transaction-repr into aad5cd3 on master.

@freddiev4 freddiev4 requested a review from richafrank Apr 12, 2017

@richafrank richafrank requested a review from ssanderson Apr 12, 2017

return """{typename}(order_id={order_id}, sid={sid}, \
dt={dt}, amount={amount}, price={price})""".format(
typename=type(self).__name__,
order_id=self.order_id,

This comment has been minimized.

@ssanderson

ssanderson Apr 17, 2017

Member

My only immediate reaction to this is that I'm not sure that it's useful to include order_id in the repr. I can't think of a case where I've wanted to see the order id when looking at a transaction.

Also, we probably want a test for this. People often forget to update a repr when removing or changing a field name, so test coverage is important for them.

This comment has been minimized.

@freddiev4

freddiev4 Apr 21, 2017

Contributor

Where would be a good place to add test coverage on this?

@ssanderson

@freddiev4 👍 for adding a repr. Had one comment and a request for test coverage.

@ssanderson

This comment has been minimized.

Member

ssanderson commented Apr 21, 2017

@freddiev4

This comment has been minimized.

Contributor

freddiev4 commented Apr 21, 2017

Updated example repr without order_id:

Transaction(sid=Asset(1), dt=2015-01-01 00:00:00, amount=10, price=50)

@freddiev4 freddiev4 force-pushed the update-transaction-repr branch from 44264c9 to 60dacec Apr 21, 2017

@coveralls

This comment has been minimized.

coveralls commented Apr 24, 2017

Coverage Status

Coverage increased (+0.002%) to 87.534% when pulling e802057 on update-transaction-repr into 4c334c6 on master.

def test_transaction_repr(self):
dt = pd.Timestamp('2017-01-01')
asset = Asset(1, exchange='test')

This comment has been minimized.

@ssanderson

ssanderson Apr 24, 2017

Member

I would probably make this an Equity or a Future. The base Asset class isn't really meant to be instantiated directly.

asset = Asset(1, exchange='test')
txn = Transaction(asset, amount=100, dt=dt, price=10, order_id=0)
expected = "Transaction(sid=Asset(1), dt=2017-01-01 00:00:00," + \

This comment has been minimized.

@ssanderson

ssanderson Apr 24, 2017

Member

You can also write a long string literal like this by putting it in parentheses, which removes the need for concatenating and escaping:

expected = (
    "Transaction(sid=Asset(1), dt=2017-01-01 00:00:00,"
    "amount=100, price=10)"
)

This comment has been minimized.

@freddiev4

freddiev4 Apr 24, 2017

Contributor

TIL. That's pretty cool 😄 I'll make that change

@@ -36,6 +36,16 @@ def __init__(self, sid, amount, dt, price, order_id, commission=None):
def __getitem__(self, name):
return self.__dict__[name]
def __repr__(self):

This comment has been minimized.

@ssanderson

ssanderson Apr 24, 2017

Member

Very minor style point: this might be slightly nicer to read broken out over multiple lines:

    def __repr__(self):
        template = "{cls}(sid={sid}, dt={dt}, amount={amount}, price={price})"
        return template.format(
            cls=type(self).__name__,
            sid=self.sid,
            dt=self.dt,
            amount=self.amount,
            price=self.price
        )

This comment has been minimized.

@ssanderson

ssanderson Apr 24, 2017

Member

Also, as a heads-up, I think sid here is going to change to asset in @jbredeche's latest (sid originally stood for "security identifier", but in modern zipline it just refers to the unique integer value assigned to every asset; in either case, the use of sid here is actually misleading, since the expected value is an Equity or Future) in #1757. Might be easiest to wait for that to merge (which should be later today) and then update here.

This comment has been minimized.

@freddiev4

freddiev4 Apr 28, 2017

Contributor

@ssanderson Updated the repr after rebasing against Jean's changes

@freddiev4 freddiev4 force-pushed the update-transaction-repr branch from e802057 to cb65fc0 Apr 28, 2017

@freddiev4 freddiev4 force-pushed the update-transaction-repr branch from cb65fc0 to e0433c4 Apr 28, 2017

@coveralls

This comment has been minimized.

coveralls commented Apr 28, 2017

Coverage Status

Coverage increased (+0.002%) to 87.712% when pulling e0433c4 on update-transaction-repr into c61fd0e on master.

@ssanderson

This comment has been minimized.

Member

ssanderson commented May 8, 2017

LGTM

@freddiev4 freddiev4 merged commit caed14a into master May 8, 2017

2 checks passed

continuous-integration/appveyor/pr AppVeyor build succeeded
Details
continuous-integration/travis-ci/pr The Travis CI build passed
Details

@freddiev4 freddiev4 deleted the update-transaction-repr branch May 8, 2017

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment