-
Notifications
You must be signed in to change notification settings - Fork 3.2k
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
[Feature] Add form_13f
Endpoint to equity.ownership
#6122
Conversation
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Thank you for adding another insightful endpoint to our Platform!
I have left some comments that cater to code quality. Also, I would kindly request to add comments in the code to indicate why that particular LOC/snippet is needed. This will make it for other developers to understand them.
openbb_platform/core/openbb_core/provider/standard_models/form_13FHR.py
Outdated
Show resolved
Hide resolved
openbb_platform/core/openbb_core/provider/standard_models/form_13FHR.py
Outdated
Show resolved
Hide resolved
openbb_platform/core/openbb_core/provider/standard_models/form_13FHR.py
Outdated
Show resolved
Hide resolved
openbb_platform/core/openbb_core/provider/standard_models/form_13FHR.py
Outdated
Show resolved
Hide resolved
Thanks for the review, I have added some comments to the |
Yes. Thank you. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Everything's fine except the warning being used to inform the source of the data. If it's necessary it must be displayed/rendered in a better way.
I would also request to update the static once again, since there was a minor tweak added.
Do we have anything documented that outlines what this is and how to use it? |
|
This is does not tell you how to inject something there, or anything at all other than a very basic definition of the field in the OBBject. |
Currently we don't have a way to do it. In the future we might if it warrants a strong need. |
So until there is a way to do it, I will continue to use warnings, because that is better than nothing. You can turn off warnings, or with |
Why must this particular endpoint among others should display the source url? What is the critical need for the user to know this? How is the user going to benefit from this information? I believe that using warnings as a workaround method to display the source url for the data is not a good practice. Warnings are supposed to convey to the user about possible issues but here this is not an issue. @IgorWounds @montezdesousa thoughts please? |
I agree that warnings are a bad place to display that. What's the practical use case for this feature at the moment? If it's not a common issue I'd wait to have a proper place to put it. |
Metadata, I cannot stress this enough and have been saying it from the beginning. This is super practical, and extremely useful - i.e., FRED series. You need to know what the units are, along with other things related to the series and you absolutely do not want to serialize this across every row of data. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
LGTM 🚀
Form 13F-HR submissions are newsworthy events, happening every calendar quarter. This is the time of year that Michael Burry remains relevant, or Berkshire Hathaway.
What? (1-3 sentences or a bullet point list):
obb.equity.ownership.form_13f()
Data is available quarterly, by calendar year, to 2013-06-30.
The
symbol
parameter accepts either, a stock ticker or CIK.The
date
parameter returns the filing for the quarter ending the date entered, and overrides thelimit
param.The
limit
parameter returns the most recent N number of reports.Impact (1-2 sentences or a bullet point list):
Not a breaking change.
Improved data coverage.
Testing Done:
Integration and unit tests added.
Variety of symbols and CIKs with limit/date combinations.
Any other information (optional)