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

DOC: Improvements on logging #2430

Merged

Conversation

willianpaixao
Copy link
Contributor

Is this feature desired, fellow maintainers?

When cleaning old bundles, I had no idea which ones were being deleted and whether it was successful or not.
A few logging lines would be appreciated.

@freddiev4
Copy link
Contributor

freddiev4 commented Feb 22, 2019

Hey @willianpaixao thanks for the PR!

Could you just post an example output for when you clean the bundles up?

@willianpaixao
Copy link
Contributor Author

Here you go:

$ zipline bundles
csvdir <no ingestions>
quandl 2019-11-10 00:45:58.577633
quandl 2019-11-10 00:34:29.738155
quandl 2019-11-10 00:26:38.237694

$ zipline clean --keep-last 1
[2019-11-10 00:53:44.868754] INFO: zipline.data.bundles.core: Cleaning 2019-11-10T00;26;38.237694.
[2019-11-10 00:53:44.909732] INFO: zipline.data.bundles.core: Cleaning 2019-11-10T00;34;29.738155.

I've just rebased my branch to upstream/master to check if there won't be any problems merging an six months old branch. All tests passed.

I'm also removing the WIP tag since I don't see any more logs to be added in this specific command (zipline clean). If you find another place to add more log messages, I would be happy to implement.

@willianpaixao willianpaixao changed the title DOC: [WIP] Improvements on logging DOC: Improvements on logging Nov 10, 2019
@willianpaixao
Copy link
Contributor Author

Hey @freddiev4 @richafrank can we get this PR merged please?

Copy link
Contributor

@ssanderson ssanderson left a comment

Choose a reason for hiding this comment

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

Hey @willianpaixao. Sorry for the long delay on this PR. I made a few small suggestions on your changes. In general, it's preferred to let the logging library to string formatting rather than doing it yourself. The main reason for this is that it avoids doing extra work for log lines that will be filtered out. In this case it's not a big deal performance-wise, but as a point of general style we try to avoid eagerly formatting log lines.

zipline/data/bundles/core.py Outdated Show resolved Hide resolved
zipline/data/bundles/core.py Outdated Show resolved Hide resolved
@willianpaixao willianpaixao force-pushed the willianpaixao_logging-improvements branch from 368e130 to 769bc8e Compare January 11, 2020 22:32
@willianpaixao
Copy link
Contributor Author

Suggestions applied, @ssanderson.

@coveralls
Copy link

coveralls commented Jan 11, 2020

Coverage Status

Coverage decreased (-0.5%) to 87.457% when pulling 769bc8e on willianpaixao:willianpaixao_logging-improvements into a1eb9b8 on quantopian:master.

@ssanderson ssanderson merged commit 16c66a4 into quantopian:master Jan 13, 2020
@ssanderson
Copy link
Contributor

thanks @willianpaixao

@willianpaixao willianpaixao deleted the willianpaixao_logging-improvements branch January 15, 2020 11:43
@ssanderson ssanderson mentioned this pull request Jan 21, 2020
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

4 participants