-
Notifications
You must be signed in to change notification settings - Fork 339
Conversation
fengshuo
commented
Oct 10, 2018
•
edited
Loading
edited
- Add capability to export tables from within the Python package (see here for more info)
- Update documentation
quandl/bulkdownloadtable.py
Outdated
from .message import Message | ||
|
||
|
||
def bulkdownloadtable(datatable_code, **kwargs): |
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.
This function should be bulk_download_table
to follow python convention.
Although now I see theres already a bulkdownload
function - maybe it would make sense to keep this name the way it is now to keep consistency.
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.
yeah i was following the bulkdownload
for timeseries data
quandl/model/datatable.py
Outdated
return file_path | ||
else: | ||
print(Message.LONG_GENERATION_TIME) | ||
self._url_request(file_or_folder_path, **options) |
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.
Correct me if I'm wrong but doesn't this line prevent the sleep(30)
from being run since it will always continue to recurse on _url_request
?
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.
ah yeah i changed the order here yesterday good catch
Theres documentation for |
quandl/model/datatable.py
Outdated
if ApiConfig.api_version: | ||
options['params']['api_version'] = ApiConfig.api_version | ||
|
||
if list(options.keys()): |
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.
Did you mean if options['params']:
?
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 updated this part since this api doesn't need to support more parameters now
quandl/model/datatable.py
Outdated
|
||
file_path = file_or_folder_path | ||
if os.path.isdir(file_or_folder_path): | ||
file_path = file_or_folder_path + '/' + code_name.replace('/', '_') + '.zip' |
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.
This will probably be better: file_path = os.path.join(file_or_folder_path, '{}.{}'.format(code_name.replace('/', '_') , 'zip')
def setUpClass(cls): | ||
httpretty.enable() | ||
httpretty.register_uri(httpretty.GET, | ||
re.compile( |
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 any reason for such formatting?
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.
not sure, I was looking at the old tests...
test/test_datatable.py
Outdated
@@ -48,3 +50,46 @@ def test_dataset_column_names_match_expected(self): | |||
metadata = Datatable('ZACKS/FC').data_fields() | |||
six.assertCountEqual(self, | |||
metadata, [u'datatable_code', u'id', u'name', u'vendor_code']) | |||
|
|||
|
|||
class BulkDownloadDataTableTest(unittest.TestCase): |
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.
Don't we need any positive test case?
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.
added one positive test case
FOR_ANALYSTS.md
Outdated
``` | ||
After the download is finished, the filename of the downloaded zip file will be returned. | ||
|
||
Sometime it takes a while to generate the zip file, you'll get a message while the file is being generated. Once the file is generated, it will start download the zip file. |
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.
Small grammar issues- should say Sometimes
instead of Sometime
. Also it will start download the
should be it will start the download of the
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.
haha, will update!
FOR_DEVELOPERS.md
Outdated
``` | ||
After the download is finished, the filename of the downloaded zip file will be returned. | ||
|
||
Sometime it takes a while to generate the zip file, you'll get a message while the file is being generated. Once the file is generated, it will start download the zip file. |
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.
Same grammar issue as I posted in the other doc file
README.md
Outdated
@@ -117,6 +117,8 @@ The following are instructions for running our tests: | |||
`python setup.py install` | |||
4. Run the following command to test the plugin in all versions of python we support: | |||
`tox` | |||
|
|||
Once you have all required package installed, you can run tests locally with `python -m unittest -v test.[test file name].[test case name].[individual test name]`. |
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.
- Should be
packages
, -v
, which stands forverbose
, is optional,- Shouldn't it be
class name
instead oftest case name
? - Also, this is to run individual tests. Users will most likely be interested in running all tests which can be accomplished by
python -m unittest
quandl/model/datatable.py
Outdated
return file_path | ||
else: | ||
print(Message.LONG_GENERATION_TIME) | ||
sleep(30) |
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 put this 30 seconds waiting time into a config file or define it as const variable with meaningful?
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 idea
quandl/model/datatable.py
Outdated
|
||
return self._url_request(file_or_folder_path, **options['params']) | ||
|
||
def _url_request(self, file_or_folder_path, **options): |
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 refactoring this method smaller and easy to read?
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.
updated
README.md
Outdated
|
||
Once you have all required package installed, you can run tests locally with: | ||
|
||
```ptyhon |
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.
spelling.
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.
😂
README.md
Outdated
@@ -117,6 +117,24 @@ The following are instructions for running our tests: | |||
`python setup.py install` | |||
4. Run the following command to test the plugin in all versions of python we support: | |||
`tox` | |||
|
|||
Once you have all required package installed, you can run tests locally with: |
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.
Should be packages
README.md
Outdated
|
||
python -W always setup.py -q test | ||
|
||
# runnging an individual test |
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.
Typo. running
README.md
Outdated
|
||
python -m unittest test.[test file name].[class name].[individual test name]` | ||
|
||
# such as |
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 Example:
is better than such as
.
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.
all updated
FOR_DEVELOPERS.md
Outdated
quandl.export_table('MER/F1', params={'compnumber': '39102', 'mapcode':'-5370','reporttype': 'A', 'qopts': {'columns': ['reportdate', 'amount']}}) | ||
``` | ||
|
||
After the download is finished, the filename of the downloaded zip file will be returned. |
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.
Are you still returning the filename? I think you changed the method to only print the filename.
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.
updated
quandl/model/datatable.py
Outdated
if 'params' not in options: | ||
options['params'] = {} | ||
|
||
while True: |
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.
status = false
while !status:
status = self._request_file_info(file_or_folder_path, **options['params'])
if !status
sleep(self.WAIT_GENERATION_INTERVAL)