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: Distinct warnings for benchmark and treasury fetchers #1971

Merged
merged 3 commits into from Oct 3, 2017

Conversation

Projects
None yet
3 participants
@JoaoAparicio
Contributor

JoaoAparicio commented Oct 2, 2017

The core of data/loader.py is the function load_market_data which
in turns calls functions ensure_benchmark_data and
ensure_treasury_data. These two functions are similar. Their
logger.info and logger.exception messages are slightly different
but their logger.warn messages are the same.

Having two different functions outputing the same warning can
be needlessly confusing when debugging.

This commit makes these two warnings explicitely different.

DOC: Distinct warnings for benchmark and treasury fetchers
The core of data/loader.py is the function load_market_data which
in turns calls functions ensure_benchmark_data and
ensure_treasury_data. These two functions are similar. Their
logger.info and logger.exception messages are slightly different
but their logger.warn messages are the same.

Having two different functions outputing the same warning can
be needlessly confusing when debugging.

This commit makes these two warnings explicitely different.
@JoaoAparicio

This comment has been minimized.

Contributor

JoaoAparicio commented Oct 2, 2017

No idea why checks failed, my changes were extremely minimal.

@freddiev4

This comment has been minimized.

Contributor

freddiev4 commented Oct 2, 2017

Hi @JoaoAparicio thanks for the PR! it looks like there was a failure coming from flake8:

image

You'll probably want to just make that log string into a multi-line string, which you can do with parentheses. I'd also probably add in the dates there. An example can be found here: https://github.com/JoaoAparicio/zipline/blob/b7bd52fdcdf1bf0c4bd7522b976324fa77a0103b/zipline/data/loader.py#L218

@JoaoAparicio

This comment has been minimized.

Contributor

JoaoAparicio commented Oct 3, 2017

@freddiev4 You were right, I had forgotten to run flake8.

Made the fixes you mentioned. In addition added dates to yet another treasury fetcher logger.info message.

@coveralls

This comment has been minimized.

coveralls commented Oct 3, 2017

Coverage Status

Coverage remained the same at 87.208% when pulling 3702693 on JoaoAparicio:loader_warnings into d79c828 on quantopian:master.

@freddiev4

This comment has been minimized.

Contributor

freddiev4 commented Oct 3, 2017

LGTM 👍 Thanks!

@freddiev4 freddiev4 merged commit 5421ec0 into quantopian:master Oct 3, 2017

2 checks passed

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

@JoaoAparicio JoaoAparicio deleted the JoaoAparicio:loader_warnings branch Oct 3, 2017

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