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 security lookup (resolves #215) #216

Merged
merged 1 commit into from
Feb 12, 2017

Conversation

Kevin-Jin
Copy link
Contributor

Implemented lookup.R and lookup.cpp.
Ran Rcpp::compileAttributes() and roxygen2::roxygenise() on new files.

It's working for me over a Bloomberg Desktop connection. Looking for feedback on style consistency, functionality, and function naming. Can probably also add some unit tests later.

Resolves #215.

Implemented lookup.R and lookup.cpp.
Ran Rcpp::compileAttributes() and roxygen2::roxygenise() on new files.
@Kevin-Jin Kevin-Jin changed the title Add support for security lookup Add support for security lookup (resolves #215) Feb 12, 2017
Copy link
Member

@eddelbuettel eddelbuettel left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This looks pretty well done at first glance.

@eddelbuettel
Copy link
Member

Builds cleanly, runs loopup("IBM") just fine.

I think we probably want a concrete example or two in the helppage. One thing that comes to mind is whether we should take advantage of R and map the yellow keys, and languages. Ie instead of

yellowKeyFilter: A character variable that restricts the asset classes
          to search in; one of “YK_FILTER_NONE”, “YK_FILTER_CMDT”,
          “YK_FILTER_EQTY”, “YK_FILTER_MUNI”, “YK_FILTER_PRFD”,
          “YK_FILTER_CLNT”, “YK_FILTER_MMKT”, “YK_FILTER_GOVT”,
          “YK_FILTER_CORP”, “YK_FILTER_INDX”, “YK_FILTER_CURR”, or
          “YK_FILTER_MTGE”

languageOverride: A character variable denoting the language that the
          results will be translated in; one of “LANG_OVERRIDE_NONE”,
          “LANG_OVERRIDE_ENGLISH”, “LANG_OVERRIDE_KANJI”,
          “LANG_OVERRIDE_FRENCH”, “LANG_OVERRIDE_GERMAN”,
          “LANG_OVERRIDE_SPANISH”, “LANG_OVERRIDE_PORTUGUESE”,
          “LANG_OVERRIDE_ITALIAN”, “LANG_OVERRIDE_CHINESE_TRAD”,
          “LANG_OVERRIDE_KOREAN”, “LANG_OVERRIDE_CHINESE_SIMP”,
          “LANG_OVERRIDE_NONE_1”, “LANG_OVERRIDE_NONE_2”,
          “LANG_OVERRIDE_NONE_3”, “LANG_OVERRIDE_NONE_4”,
          “LANG_OVERRIDE_NONE_5”, “LANG_OVERRIDE_RUSSIAN”

have something like yellowkey <- c("none", "cmdt", "eqty", "muni", "prfd", "clnt", "mmkt", "govt", "corp", "indx", "curr", "mtge") and then a match.arg() on it. Ditto for the languages.

I can just add that.

@eddelbuettel eddelbuettel merged commit 30aeecb into Rblp:master Feb 12, 2017
@Kevin-Jin
Copy link
Contributor Author

Kevin-Jin commented Feb 12, 2017

Agreed, that would look much nicer and be more consistent with R standards.
One thing I also realized is that one of us should probably rename the function to lookupSecurity() in case someone ever wants to implement lookupCurve() or lookupGovt(). lookup() is also a rather undescriptive name and probably would clash with a function in another namespace.
Alternatively we could just copy the interface of the Matlab function exactly. We would then have to let the user specify one of "instrumentListRequest", "curveListRequest", or "govtListRequest" and accept yellowKeyFilters, languageOverrides, etc. in a named list of options.

@eddelbuettel
Copy link
Member

Yes, I am half-way done with a new PR [but got called away for a few minuttes] doing that, and I agree on the renaming need.

And yes, lookupSecurity() works for me too.

@eddelbuettel
Copy link
Member

I have it working, but am not sure I have language in effect. Do you have a sample query where it matters, and result come back in something I can still read (ie French or German) or an alphabet that is clearly different?

@Kevin-Jin
Copy link
Contributor Author

Unfortunately no, I forgot to test that part and I won't have access to a Bloomberg terminal until tomorrow. The Matlab function, the Bloomberg Developer API sources, Bloomberg's SecurityLookupExample, and this dump of the definition were what led me to include that option but I haven't actually tested its functionality.

@eddelbuettel
Copy link
Member

I fear we do have Matlab with that API to borrow ideas from. But this was a very, very useful one. I too will continue testing this tomorrow. For now I'm going to make it a new PR.

But that was a really useful, and well-done PR. This is exactly how I think open source works at its best: we gave you a working platform to work on and with, and you made it better.

(If by the way you think you can improve build instructions etc pp that might be helpful too. We could at least punch some things into the wiki.)

@eddelbuettel
Copy link
Member

Oh, and also in favour of harvesting StackOverflow for other good ideas. Well played on that front too.

@Kevin-Jin
Copy link
Contributor Author

Kevin-Jin commented Feb 12, 2017

Thanks! I really appreciate how promptly you maintain the package. I'm working on a project that needs this function and hopefully this'll be released to CRAN by the time I have to share my project in a couple of months. Building on Windows is a PITA unfortunately because of upstream issues.

@ucb
Copy link

ucb commented Feb 28, 2017

Great! It is nice to have it native in the package. The workaround I have been using is the exe of a cpp code from Bloomberg, included with the standard API installation. Calling this exe from R and then reading the output.

This approach can also be used for testing of the new function, imho.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants