Skip to content

Conversation

@arnonm
Copy link
Contributor

@arnonm arnonm commented Oct 15, 2025

Replaces #5079 with a single Commit

@Nirus2000
Copy link
Member

Nirus2000 commented Oct 15, 2025

Hello @arnonm
Thank you for the pull request.
Unfortunately, I can't wrap my head around committing 350k of source code for a price provider. Especially since a lot of the source code is uncommented, and comments are incorrect or wrong.
Some to-dos, etc.
I don't think anyone will take the time for this pull request. We have also implemented other course providers, such as Leeway, Yahoo, etc., and they are more resource-efficient.
Also... the tests fail...

@arnonm
Copy link
Contributor Author

arnonm commented Oct 16, 2025

So addressing the comments. Removed the live API tests and the sample files, which reduced the commit to 11,000 lines. 5000 of which are in 2 mock response files. I can cut these as well.
Will do a review of the comments and ensure to clean up the comments.

@buchen
Copy link
Member

buchen commented Oct 20, 2025

Hi @arnonm,

thanks for the contribution.

First, I am trying to understand which API endpoints this contribution is using and which terms and conditions apply.

I found these general terms of service linked on the footer of the web page. It makes no mention of an API but also does not prohibit the use of the data for personal purposes. I see there is also a "TASE Data Hub" with API documentation and an API key for authentication. There are other data providers where one can configure the API key in the settings. I wasn't able to figure out any details now, but it sounds like a reasonable option?

Second, on a first glance I noticed a couple things with the pull request. One issue is that the complete listing is downloaded upon the start of PP. Why? The constructor of the Feed is loading the data but feeds are constructed at the start of PP. That means tens of thousands of downloads per day. My proposal is that we unpack this contribution: first have the download of historical prices if the user has configured TAVE as a source. In a second step, we extend the search in a reasonable way (either search TAVE only if user has provided an API key, or some other way). With the search, I first have to better understand what kind of traffic would go to the side.

@arnonm
Copy link
Contributor Author

arnonm commented Oct 23, 2025 via email

@arnonm
Copy link
Contributor Author

arnonm commented Nov 11, 2025

Added a check so that getAllIndices is not kicked off on Construction, only when the first search is called, and added a test to ensure it happens

@arnonm
Copy link
Contributor Author

arnonm commented Nov 24, 2025

@buchen - raising this to the top again..

@arnonm
Copy link
Contributor Author

arnonm commented Dec 8, 2025

@buchen - anything blocking this?

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Projects

Status: No status

Development

Successfully merging this pull request may close these issues.

3 participants