search

braverock / blotter

blotter provides transaction infrastructure for defining transactions, portfolios and accounts for trading systems and simulation. Provides portfolio support for multi-asset class and multi-currency portfolios. Actively maintained and developed.
114 stars 50 forks source link

update call to DailyEqPL/DailyTxnPL #29

Closed jaymon0703 closed 8 years ago

jaymon0703 commented 8 years ago

adding incl.total = TRUE param for DailyEqPL and DailyTxnPL functions, as well as subsetting dailyPL variable to be single column xts of portfolio PL

tested on Joshua's turtles demo inside the blotter package

joshuaulrich commented 8 years ago

This cannot be merged cleanly because @braverock made changes that advanced master ahead of the commit where you created the branch containing this pull request. In the future, you should make sure your local copy is up to date with the remote (git fetch and/or git pull --rebase) before submitting a pull request.

Also note that both of you changed the function signature. You removed ... and @braverock moved ... to the middle of the function arguments in 632587e11261e078e824ca15b4511728d6a802d2. I can merge this PR, but I need to know the correct parameters and their order.

jaymon0703 commented 8 years ago

Thanks Josh noted, will let Brian confirm the ... param and their order next week.

Sent from my iPhone

On 17 Jun 2016, at 4:28 AM, Joshua Ulrich notifications@github.com<mailto:notifications@github.com> wrote:

This cannot be merged cleanly because @braverockhttps://github.com/braverock made changes that advanced master ahead of the commit where you created the branch containing this pull request. In the future, you should make sure your local copy is up to date with the remote (git fetch and/or git pull --rebase) before submitting a pull request.

Also note that both of you changed the function signature. You removed ... and @braverockhttps://github.com/braverock moved ... to the middle of the function arguments in 632587ehttps://github.com/braverock/blotter/commit/632587e11261e078e824ca15b4511728d6a802d2. I can merge this PR, but I need to know the correct parameters and their order.

— You are receiving this because you authored the thread. Reply to this email directly, view it on GitHubhttps://github.com/braverock/blotter/pull/29#issuecomment-226665423, or mute the threadhttps://github.com/notifications/unsubscribe/AEI5vKpLGqUclcA8sLIR9EF3pNrzkPTkks5qMgYqgaJpZM4I3sHh.

This email and any attachment is confidential. If you are not the intended recipient, please delete this message. Macquarie does not guarantee the integrity of any emails or attachments. For important disclosures and information about the incorporation and regulated status of Macquarie Group entities please see: www.macquarie.com/disclosures

braverock commented 8 years ago

I moved the dots to earlier in the function signature because parameters after dots must be fully matched.

We'll be adding other methods, which means more arguments, and the order of arguments might get rearranged for clarity

If they're any new arguments are after dots, we can add or rearrange (but not rename) arguments without breaking any user code that uses the function.

joshuaulrich commented 8 years ago

Merged in 1b22605cc9626ade9bab6c68c9e2f6adacef93df.