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

Refactor Security to Asset #535

Merged
merged 2 commits into from Mar 19, 2015
Merged

Refactor Security to Asset #535

merged 2 commits into from Mar 19, 2015

Conversation

@jfkirk
Copy link
Contributor

@jfkirk jfkirk commented Mar 16, 2015

This commit refactors the Security cython class to Asset, and refactors some fields of the class accordingly. This change is so the terminology is consistent and correct when Asset is extended to asset types that are not securities, such as futures.

@jfkirk jfkirk assigned jfkirk and ehebert and unassigned jfkirk Mar 16, 2015
@llllllllll
Copy link
Contributor

@llllllllll llllllllll commented Mar 16, 2015

👍 on clear naming

@ehebert
Copy link
Contributor

@ehebert ehebert commented Mar 16, 2015

The tests pass locally for me as well, I just kicked off a new Travis build to see if the error is repeatable.

@ehebert
Copy link
Contributor

@ehebert ehebert commented Mar 16, 2015

Besides the tests, this looks good to me.

Perhaps also add Security to the module as a subclass to show how it would be used as a base?

@jfkirk
Copy link
Contributor Author

@jfkirk jfkirk commented Mar 16, 2015

@ehebert Will do. This PR should not be merged until that has been added.

@jfkirk
Copy link
Contributor Author

@jfkirk jfkirk commented Mar 17, 2015

@ehebert Added extensions for Equity and Future, as we've discussed. I also added a test for the Future class that checks that the fields are assigned properly and reported through the repr. Thanks so much for all of your help.

jfkirk added 2 commits Mar 16, 2015
This commit refactors the Security cython class to Asset, and refactors some fields of the class accordingly. This change is so the terminology is consistent and correct when Asset is extended to asset types that are not securities, such as futures.
@jfkirk jfkirk force-pushed the rename-security-to-asset branch from 72ecf4f to 84d4fa3 Mar 19, 2015
jfkirk added a commit that referenced this pull request Mar 19, 2015
Refactor Security to Asset
@jfkirk jfkirk merged commit 39f3b94 into master Mar 19, 2015
3 checks passed
3 checks passed
Scrutinizer 2 updated code elements
Details
continuous-integration/travis-ci/pr The Travis CI build passed
Details
continuous-integration/travis-ci/push The Travis CI build passed
Details
@jfkirk jfkirk deleted the rename-security-to-asset branch Mar 23, 2015
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Linked issues

Successfully merging this pull request may close these issues.

None yet

3 participants
You can’t perform that action at this time.