Skip to content
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

add support for portfolio data (closes #169) #176

Merged
merged 2 commits into from Jun 6, 2016
Merged

Conversation

@johnlaing
Copy link
Contributor

@johnlaing johnlaing commented Jun 6, 2016

No description provided.

@eddelbuettel
Copy link
Member

@eddelbuettel eddelbuettel commented Jun 6, 2016

Looks great. Only minor question (or suggestion): should it be getPortfolio() for consistence as it is a getter? Or not because bds, bdp, bdh, ... are shorthand?

@johnlaing
Copy link
Contributor Author

@johnlaing johnlaing commented Jun 6, 2016

I don't have a preference either way. Happy to change if you'd like.

@eddelbuettel
Copy link
Member

@eddelbuettel eddelbuettel commented Jun 6, 2016

As a user (and author) I do prefer getFoo() for various foo, so if you're up for a search+replace I think it makes it easier. But as I said, here we do have counter examples -- and if the Bloomberg documentation refers to 'portfolio' as a function ... So your call.

@johnlaing
Copy link
Contributor Author

@johnlaing johnlaing commented Jun 6, 2016

I'll change it. I like bdp/bds/bdh for their simplicity, but there are others like getTicks. I suppose this is more similar to the latter.

@armstrtw armstrtw merged commit f764f0e into master Jun 6, 2016
2 checks passed
2 checks passed
continuous-integration/travis-ci/pr The Travis CI build passed
Details
continuous-integration/travis-ci/push The Travis CI build passed
Details
@wmorgan85 wmorgan85 mentioned this pull request Jul 12, 2016
@eddelbuettel eddelbuettel deleted the feature/portfolio-data branch Sep 16, 2016
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Linked issues

Successfully merging this pull request may close these issues.

None yet

3 participants
You can’t perform that action at this time.