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.
112 stars 49 forks source link

chart.ME : add legend.loc as a new parameter #77

Closed anshul96go closed 5 years ago

anshul96go commented 5 years ago

Solved issue #66

braverock commented 5 years ago

I'm happy to consider a patch in this direction, but we should be consistent in the naming conventions and functionality for plot and plot.xts

So the parameter should be legend.loc as in plot.xts

From the plot.xts documentation:

legend.loc: places a legend into one of nine locations on the chart:
            bottomright, bottom, bottomleft, left, topleft, top,
            topright, right, or center. Default NULL does not draw a
            legend.
anshul96go commented 5 years ago

@braverock Should the working of parameter also remain the same? I was thinking of giving it bottomright value rather than NULL

braverock commented 5 years ago

I think a default of bottomright is fine. The documentation would need to reflect this default and the NULL option (no legend)

anshul96go commented 5 years ago

@braverock
Please verify the last commit

anshul96go commented 5 years ago

@braverock Should I start working on the application for GSOC and which channel should I use for communication?

jaymon0703 commented 5 years ago

Thanks @anshul96go this PR is merged

braverock commented 5 years ago

@jaymon0703 has merged this pull request. Thanks @anshul96go and @vi-to for the input.

This closes #77 and #66

vi-to commented 5 years ago

Hi, thank you very much for the feedback! About the pull request just merged, I would like to point out that it does not allow to plot chart.ME without a legend, that is to set legend.loc = NULL. It may be useful to do so because there may be trade points hidden by the legend. This option was allowed in pull request #78. Clearly, it is okay if you consider it to be a minor point and do not want it, the present remarks are just for the sake of development.

braverock commented 5 years ago

@vi-to thanks for your comment.

I encourage you to open a minimal (no whitespace, no extraneous stuff) pull request with a clear and useful commit message, and we would be happy to test and merge it.

It usually isn't a good idea to have dueling commits, and one of the challenges of your pull request was that it would have been better structured as a branch off the #77 pull request, so that everything would stay organized and traceable.