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

BUG: Fail fast on invalid pipeline columns #1280

Merged
merged 2 commits into from Jun 22, 2016

Conversation

Projects
None yet
3 participants
@dmichalowicz
Contributor

dmichalowicz commented Jun 15, 2016

If you try to run a pipeline with a column of USEquityPricing.close (or something similar) and you meant to use USEquityPricing.close.latest, it errors in a non-helpful/obvious way.

@coveralls

This comment has been minimized.

coveralls commented Jun 15, 2016

Coverage Status

Coverage increased (+0.005%) to 79.785% when pulling 6b9b9fb on bad-pipeline-columns into 4c2f0e8 on master.

@@ -37,6 +37,12 @@ def __init__(self, columns=None, screen=None):
if columns is None:
columns = {}
for term in columns.values():

This comment has been minimized.

@ssanderson

ssanderson Jun 16, 2016

Member

Showing the name of the column here would be nice as well.

@@ -54,7 +60,7 @@ def screen(self):
"""
return self._screen
@expect_types(term=Term, name=str)
@expect_types(term=ComputableTerm, name=str)

This comment has been minimized.

@ssanderson

ssanderson Jun 16, 2016

Member

I would try to re-use the error message above rather than using expect_types here. This will spit out a message telling the user that we expected a ComputableTerm but got a BoundColumn, which won't mean anything to most users.

raise TypeError(
"{term} is not a valid pipeline column. Did you mean to "
"append '.latest'?".format(term=term)
)

This comment has been minimized.

@dmichalowicz

dmichalowicz Jun 17, 2016

Contributor

Added the column name above but not here. In this case you don't need the name as you're only adding a single term.

EDIT: Typo

@ssanderson

This comment has been minimized.

Member

ssanderson commented Jun 21, 2016

👍 LGTM

@ssanderson ssanderson merged commit e510cbb into master Jun 22, 2016

2 checks passed

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

@ssanderson ssanderson deleted the bad-pipeline-columns branch Jun 22, 2016

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