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

Removed the __validate_exchange method since it doesn't allow to manage all supported exchanges #35

Merged
merged 2 commits into from
May 22, 2023

Conversation

enricodetoma
Copy link
Contributor

@enricodetoma enricodetoma commented May 21, 2023

The hard-encoded list of exchanges is too narrow.
For example the Italian stock exchange MIL is not included.
I propose to remove the __validate_exchange method completely and let the FMP api themselves manage a non-existing exchange by returning 0 results.

In addition I found a small bug in the stock_screener API: parameters is_etf and is_actively_trading must be checked for None, not for True/False only. Previously, for example, if you specified is_etf=False, it would return both ETFs and stocks.

@ipl31
Copy link
Contributor

ipl31 commented May 22, 2023

Overall change looks good to me. Good catch on the is_etf is_actively_trading issue. This PR is opened against dev, I think it should be opened against master.

It would be nice if you could squash the two commits for removing __validate_exchange into a single commit. Also

@enricodetoma
Copy link
Contributor Author

enricodetoma commented May 22, 2023

I squashed the commits.

The reason I made the PR against dev instead of master is because I noticed that the released version on https://pypi.org/project/fmpsdk/ is version 20230312.0 which corresponds to dev branch, not master:

master...dev

Please confirm if I should instead make the pull request against master.

@daxm daxm merged commit feb914e into daxm:dev May 22, 2023
@daxm
Copy link
Owner

daxm commented May 22, 2023

Alas, I wanted to just merge the _is_etf part of the commit but grabbed both. Let's see how it goes.

The reason I didn't want to do the first commit is because I want this tool to also be helpful to the scripters using it. Not everyone is going to know what is a valid string for the Exchanges supported. I'd rather us maintain a list here and provide useful feedback to the user than just blindly pass through whatever string they send.

I'd like to see/hear more discussion on this if anyone has opinions they wish to share.

For now I'll add this in but may in the future revert to validating the string sent.

Also, I didn't notice that I had made the last pip update from 'dev'. I'll fix that now.

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.

3 participants