-
Notifications
You must be signed in to change notification settings - Fork 9
Adding functionality for Financials API #2
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
base: master
Are you sure you want to change the base?
Conversation
godber
left a comment
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.
Please consider the two comments I've made. The first is a simple accept my suggested changes.
I am not the author of the original library so I don't know that this will improve the chances of getting this merged, but it looks good.
To the original author of the module:
I have reviewed the contents of this PR to ensure it doesn't contain anything suspicious, I even skimmed through the newly added financials.py file to make sure it didn't have any suspicious code.
I have not RUN these changes though.
| **bankfind** is a python interface to publically available bank data from the FDIC. | ||
|
|
||
| There are currently, as of 8/11/20, five endpoints that the FDIC has exposed to the public: | ||
| There are currently, as of 4/15/2023, five endpoints that the FDIC has exposed to the public: |
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.
| There are currently, as of 4/15/2023, five endpoints that the FDIC has exposed to the public: | |
| There are currently, as of 4/15/2023, six endpoints that the FDIC has exposed to the public: |
| search: str = None, | ||
| **kwargs): | ||
| params = self._construct_params(key, filters, search, **kwargs) | ||
| print(urllib.parse.urlencode(params)) |
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.
Do you really mean to leave this print statement in here? If this project already has a logging facility it might be best to use that or hide this print statement behind a debug environment variable or something.
|
This would close #1 |
|
I've had two other thought here:
@dpguthrie do you have interest in accepting contributions like this PR or are you more interested in us forking? |
The FDIC released the financials API after dpguthrie first published this module. I added in the ability to access that API using the same functionality as the Bankfind suite. Because I found Bankfind so helpful, I wanted to provide my changes in case others found them useful.