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 ETF saving function #14

Merged
merged 3 commits into from
Nov 10, 2023
Merged

Conversation

larrysammii
Copy link
Contributor

I found digging into the venv's library files to get the JSON file a bit cumbersome, so I quickly did some minor changes and defined a new function that could be optionally imported into user's code.

When using terminal script, it should remain the same except the JSON file is now stored at pyetf/etfpy.

When imported, the location of the saved file can be specified by the file_path parameter. If no location is specified, the file will be saved to the project root directory.

@JakubPluta
Copy link
Owner

Hey @larrysammii, thank you for your PR, it was something that I fortget to take care off.

One thing that I noticed, that the file etfpy/data/etfs/etfs_list.json is empty.
I know that you added new functionality to specify where you want to store scraped etfs list, but this file etfpy/data/etfs/etfs_list.json is used internally by other code components.
The list of etfs changes very rearly, so I diceded to put this list in this folders, so this way if user want to investigate/analyze single etf first there is a check (if etf ticket exists in this file)
Probably I will add some scheduled job that once a month it scrapes all etfs list and redeployd the package.

So for now, I would suggest to just leave etfpy/data/etfs/etfs_list.json file with the data, and the rest should be okey

@larrysammii
Copy link
Contributor Author

Apologies for the error @JakubPluta , I just restored the content within the etfs_list.json.

Copy link
Owner

@JakubPluta JakubPluta left a comment

Choose a reason for hiding this comment

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

Everything good now :) Thanks

@JakubPluta JakubPluta merged commit 95c0389 into JakubPluta:main Nov 10, 2023
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.

None yet

2 participants