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

[Feature]try to add fmp senate trading data api #6957

Merged

Conversation

joshuaBri
Copy link
Contributor

  1. Why?:

try to add fmp senate trading data api
2. What?:

attempted to integrate the Senate API

@CLAassistant
Copy link

CLAassistant commented Nov 22, 2024

CLA assistant check
All committers have signed the CLA.

@markqiu
Copy link

markqiu commented Nov 27, 2024

@mmistroni take a look please.

@mmistroni
Copy link
Contributor

mmistroni commented Nov 27, 2024 via email

@deeleeramone deeleeramone self-requested a review November 28, 2024 03:19
@deeleeramone deeleeramone added enhancement Enhancement platform OpenBB Platform v4 PRs for v4 labels Nov 28, 2024
Copy link
Contributor

@deeleeramone deeleeramone left a comment

Choose a reason for hiding this comment

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

Thanks for the PR, this is great!

I'm asking for a few minor changes, and to please translate all code comments into English.

Thanks very much!

Copy link
Contributor Author

@joshuaBri joshuaBri left a comment

Choose a reason for hiding this comment

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

@deeleeramone Thanks for your correction, I alreadey improved my codes.

@deeleeramone
Copy link
Contributor

deeleeramone commented Dec 1, 2024

@deeleeramone Thanks for your correction, I alreadey improved my codes.

We still need to clear all the empty strings and sort the output by date, doing so will reveal problems with the data model definitions. Symbol is not always going to be reported because it might not be a stock. It will need to be Optional. Perhaps the other mandatory field should be "representative"?

Here's what happens:

    return sorted(
            [
                FMPGovernmentTradesData(
                    **{k: v for k, v in d.items() if v and v != "--"}
                )
                for d in data
            ],
            key=lambda x: x.date,
            reverse=True,
        )
OpenBBError: 
[Error] -> 1 validations error(s)
[Data Model] FMPGovernmentTradesData
[Arg] symbol -> input: {'date': '2024-11-21', 'transaction_date': '2024-11-04', 'owner': 'Joint', 'assetDescription': 'TX Trans Commn TPK 5% TPK Tran Due 08/15/42', 'type': 'Purchase', 'amount': '$1,001 - $15,000', 'representative': 'Suzan K. DelBene', 'link': 'https://disclosures-clerk.house.gov/public_disc/ptr-pdfs/2024/20026238.pdf'} -> Field required

@joshuaBri
Copy link
Contributor Author

@deeleeramone Thanks for your correction, I alreadey improved my codes.

We still need to clear all the empty strings and sort the output by date, doing so will reveal problems with the data model definitions. Symbol is not always going to be reported because it might not be a stock. It will need to be Optional. Perhaps the other mandatory field should be "representative"?

Here's what happens:

    return sorted(
            [
                FMPGovernmentTradesData(
                    **{k: v for k, v in d.items() if v and v != "--"}
                )
                for d in data
            ],
            key=lambda x: x.date,
            reverse=True,
        )
OpenBBError: 
[Error] -> 1 validations error(s)
[Data Model] FMPGovernmentTradesData
[Arg] symbol -> input: {'date': '2024-11-21', 'transaction_date': '2024-11-04', 'owner': 'Joint', 'assetDescription': 'TX Trans Commn TPK 5% TPK Tran Due 08/15/42', 'type': 'Purchase', 'amount': '$1,001 - $15,000', 'representative': 'Suzan K. DelBene', 'link': 'https://disclosures-clerk.house.gov/public_disc/ptr-pdfs/2024/20026238.pdf'} -> Field required

I apologize for not uploading the sorting part of the code earlier. I've tested it elsewhere and realized that duplicate fields and the possibility of 'symbol' being empty were causing standard class validation errors.
image
And clearing all the empty strings is inneed,too. I understand now and am correcting them. Thank you again for your correction.

@deeleeramone deeleeramone self-requested a review December 7, 2024 06:34
Copy link
Contributor

@deeleeramone deeleeramone left a comment

Choose a reason for hiding this comment

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

Thank you very much for your contribution, excellent work!

I made some small changes for linters and repo consistency. I also discovered that the last page in the RSS feeds is 30, so I capped the requests from happening beyond that point. FMP includes bad requests in their limits.

@joshuaBri, you need to do this before the PR can be merged, thanks!

Copy link
Contributor

@IgorWounds IgorWounds left a comment

Choose a reason for hiding this comment

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

Thank you most kindly for your contribution!

@deeleeramone deeleeramone added this pull request to the merge queue Dec 7, 2024
@deeleeramone deeleeramone removed this pull request from the merge queue due to a manual request Dec 7, 2024
@deeleeramone deeleeramone added this pull request to the merge queue Dec 10, 2024
Merged via the queue into OpenBB-finance:develop with commit d704de5 Dec 10, 2024
10 checks passed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
enhancement Enhancement platform OpenBB Platform v4 PRs for v4
Projects
None yet
Development

Successfully merging this pull request may close these issues.

6 participants