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

MAINT: Remove future_chain API method. #1502

Merged
merged 1 commit into from Sep 21, 2016
Merged

MAINT: Remove future_chain API method. #1502

merged 1 commit into from Sep 21, 2016

Conversation

ehebert
Copy link
Contributor

@ehebert ehebert commented Sep 20, 2016

future_chain will be replaced by the as yet to be implemented method,
data.current_chain

Also removing FutureChain which will be replaced by another version
which only supports indexing and iteration.

12: 'Z', # December
}
for key in codes:
self.assertEqual(codes[key], month_to_cme_code(key))
Copy link
Contributor

Choose a reason for hiding this comment

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

Since we're not removing cme_code_to_month or month_to_cme_code, should we maybe relocate these two tests?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Open to adding them back in. Had removed them because it was essentially testing whether a dict is behaving as expected. (And the unused functions may be an unnecessary step. Following the pattern in other places where we refer to definitions, the definitions could be used as CME_CODE_TO_MONTH[code] and MONTH_TO_CME_CODE[code].

Copy link
Contributor

Choose a reason for hiding this comment

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

+1 on removing the functions.

@@ -295,15 +295,6 @@ class SymbolNotFound(ZiplineError):
""".strip()


class RootSymbolNotFound(ZiplineError):
Copy link
Contributor

Choose a reason for hiding this comment

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

We'll probably still need something like this, right?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Probably, can change patch so that it remains.

`future_chain` will be replaced by the as yet to be implemented method,
`data.current_chain`

Also removing `FutureChain` which will be replaced by another version
which only supports indexing and iteration.
@coveralls
Copy link

coveralls commented Sep 21, 2016

Coverage Status

Coverage decreased (-0.07%) to 86.544% when pulling f4daf10 on remove-future-chain into f3c4381 on master.

@ehebert ehebert merged commit 9f77473 into master Sep 21, 2016
@ehebert ehebert deleted the remove-future-chain branch September 21, 2016 15:45
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

3 participants