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

Register blotter instances not types #2251

Merged
merged 6 commits into from Jul 16, 2018

Conversation

Projects
None yet
3 participants
@ssanderson
Member

ssanderson commented Jul 16, 2018

No description provided.

ssanderson added some commits Jul 16, 2018

ENH: Register arbitrary factories.
Instead of requiring users to register types, support registration of an
arbitrary function returning an instance of the appropriate type.

@ssanderson ssanderson requested review from llllllllll and jnazaren Jul 16, 2018

@llllllllll

One question about the new behavior for load, otherwise lgtm

@@ -106,17 +100,13 @@ class ProperDummyInterface(FakeInterface):
class Fake(object):
pass
msg = "FakeInterface class 'ayy-lmao' is already registered"
with assert_raises_str(ValueError, msg):
m = "FakeInterface factory with name 'ayy-lmao' is already registered"

This comment has been minimized.

@llllllllll

llllllllll Jul 16, 2018

Member

does this keep this line short enough to fit on one line?

This comment has been minimized.

@ssanderson
'--blotter-class',
type=click.Choice(list(get_registry(Blotter).get_registered_classes())),
'--blotter',
type=str,

This comment has been minimized.

@llllllllll

llllllllll Jul 16, 2018

Member

does this do anything?

This comment has been minimized.

@ssanderson

ssanderson Jul 16, 2018

Member

Probably not. Will remove.

try:
blotter_class = load(Blotter, blotter_class)
blotter = load(Blotter, blotter)()

This comment has been minimized.

@llllllllll

llllllllll Jul 16, 2018

Member

should load resolve to the blotter? What do we do with metric sets?

This comment has been minimized.

@ssanderson

ssanderson Jul 16, 2018

Member

load on metric sets and bundles returns the metric set / bundle. I updated this to return an instance rather than the type.

ssanderson added some commits Jul 16, 2018

MAINT: Return objects from Registry.load.
- Rather than returning types, return fully constructed objects by calling
  the registered factory functions.

- Rename more references to ``class`` in Registry.
STY: Remove unused imports.
And fix old-style class in py2.
self.blotter = kwargs.pop('blotter', None)
self.cancel_policy = kwargs.pop('cancel_policy', NeverCancel())
cancel_policy = kwargs.pop('cancel_policy', NeverCancel())

This comment has been minimized.

@richafrank

richafrank Jul 16, 2018

Member

Would we want to make this conditional on not self.blotter below?

This comment has been minimized.

@ssanderson

ssanderson Jul 16, 2018

Member

I did this originally, but doing so breaks our consumers downstream because we expect this kwarg to always get popped. I've made substantial changes to the TradingAlgorithm constructor in #2192, so my inclination is to punt on cleaning this up until after that refactor langds.

This comment has been minimized.

@richafrank

richafrank Jul 16, 2018

Member

Got it. Alrighty.

@ssanderson ssanderson merged commit 9bfade9 into master Jul 16, 2018

2 checks passed

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

@ssanderson ssanderson deleted the register-blotter-instances-not-types branch Jul 16, 2018

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