-
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
put everything into packages #23
Conversation
numismatic/cli.py
Outdated
@@ -10,6 +10,8 @@ | |||
from streamz import Stream | |||
import attr | |||
|
|||
from numismatic.feeds import * |
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.
Is there a way to import into another namespace instead of the global namespace when using from ... import *
?
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 just tried to do this inside the Exchange.factory() method but got an error saying that * imports are only allowed at the module level.
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.
Interestingly, you can do this: exec('from ... import *')
- that won't throw errors, but I don't know what namespace it uses because I can't find the imported things.
import math | ||
from datetime import datetime, timedelta | ||
|
||
def date_range(start_date, end_date, **freq): |
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.
Could you check that there is no bug with the **freq
argument in date_range
?
state['cache_dir'] = cache_dir | ||
state['datafeed'] = \ | ||
Datafeed.factory(feed_name=feed, cache_dir=cache_dir, | ||
Feed.factory(feed_name=feed, cache_dir=cache_dir, |
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.
If one could import into a namespace, then I would be able to say feed.base.factory...
@@ -166,7 +168,6 @@ def listen(state, exchange, assets, currencies, raw_output, batch_size, | |||
channel, api_key_id, api_key_secret): | |||
'Listen to live events from an exchange' | |||
# FIXME: Use a factory function here | |||
from .exchanges import BitfinexExchange, LunoExchange | |||
assets = ','.join(assets).split(',') |
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.
Lastly, importing into a namespace would mean that we don't have to name this BitfinexExchange - instead, we would call it exchanges.Bitfiniex...
BTW: I agree with your comment that we need a factory...
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 added a factory method for this.
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.
It's not as useful yet as the API/signatures aren't consistent across the exchanges yet but it's a start.
Thank you Barry this looks great. I wanted to review your comments and perhaps push up some changes so I created a |
@barrysteyn I don't know what the best way to collaborate on a PR is? I put this in a branch in my repo so I could track your changes and I've made some commits. I would like those to show up in this thread here on github. Otherwise if that's not possible then I'll just merge your changes and merge my changes on top of that. |
It looks like |
LGTM |
Thank you @barrysteyn |
This creates packages and generally neatens the folder structure. It addresses #21 and #15