-
Notifications
You must be signed in to change notification settings - Fork 14
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
Bugz collector #62
Bugz collector #62
Conversation
@@ -262,39 +262,43 @@ def subscribe(state, feed, exchange, assets, currencies, interval, channels): | |||
|
|||
|
|||
@coin.command() | |||
@click.option('--market', '-m', default='all') |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Good call. I thought this was getting too complicated.
type=click.Choice(['None', 'Trade', 'Heartbeat', 'LimitOrder', | ||
'CancelOrder'])) | ||
@click.option('--filters', '-f', default=None, type=str, multiple=True) | ||
# FIXME: the types should be autogenerated from the Event types |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
That would be cool... but it may also seem like magic...
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Perhaps, but we currently do the same for Feeds and Collectors. That's one of the nice things because you just have to register a new Feed and the CLI help output updates automatically.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I know, that also seems like magic. The things I have against magic is that takes more time to understand how the infrastructure works as opposed to what it does. This is not to say it's always bad, but often times, it leads to over engineering...
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
That's true.
@click.option('--text', 'format', flag_value='text', default=True) | ||
@click.option('--json', 'format', flag_value='json') | ||
@click.option('--interval', '-i', default=None, type=float) | ||
@pass_state | ||
def collect(state, market, stream, collector, filter, type, output, format, interval): | ||
def collect(state, subscriptions, stream, collector, output, filters, types, format, |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Collect sure has a long argument list...
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
LGTM
This is working nicely now on Poloniex, and I definitely noticed this problem in the past
The problem was that at some point I changed the order of the decorators but hadn't adjusted the order of the arguments so options were sent to the wrong arguments. |
Fixes Collector handling of filters and types and adds pattern matching for subscriptions.
@barrysteyn please review