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

Add config #2209

Closed
wants to merge 3 commits into from
Closed

Add config #2209

wants to merge 3 commits into from

Conversation

R5dan
Copy link
Contributor

@R5dan R5dan commented Jan 6, 2025

Adds a new Config class with defaults defined

Allows setting of:

  • language
  • proxy
  • request url
  • session
  • region

Use is global so don't need to set them separately everywhere (for example you most likely would use proxy everywhere and now you just set it once)

Allows creation of custom Configs and to set them as default.

Currently is a draft as only Ticker.history and download have it implemented

@R5dan R5dan marked this pull request as draft January 6, 2025 14:59
@R5dan
Copy link
Contributor Author

R5dan commented Jan 7, 2025

@ValueRaider what do you think? This would help with issues like #2210

@ValueRaider
Copy link
Collaborator

ValueRaider commented Jan 7, 2025

The quantity of code changes concerns me - code bloat. Can it be done with less? I had already been thinking of a related solution for network settings but more like this:

yf.set_network(session=X, proxy=Y)
# which passes directly through to YfData singleton
YfData._set_session(X)
YfData._set_proxy(Y)

20 LoC for everything? And consider relaxing on the docstrings - set_session is obvious, it sets the session.

(I've not missed lang and region but that is another discussion, let's find agreement on network stuff first)

Merge branch 'dev' into config

This will block you squashing commits. Better to rebase.

@R5dan
Copy link
Contributor Author

R5dan commented Jan 8, 2025

The quantity of code changes concerns me - code bloat

Most of that is Merge branch 'dev' into config and updating the inputs (so there isnt an option for proxy or timeout just config

Merge branch 'dev' into config

This will block you squashing commits. Better to rebase.

Yes, I realize that, I was given a button to "update" the pr as I had forgotten to pull the latest dev version, but it didn't do what I thought it would. Will make a new PR if you are happy.

@R5dan
Copy link
Contributor Author

R5dan commented Jan 8, 2025

yf.set_network(session=X, proxy=Y)
# which passes directly through to YfData singleton
YfData._set_session(X)
YfData._set_proxy(Y)

Hadn't thought of this method, I would say this makes it harder to expand it though, and doesn't allow for multiple configs for different queries.

I will remove most of the docstrings

@ValueRaider
Copy link
Collaborator

ValueRaider commented Jan 8, 2025

doesn't allow for multiple configs for different queries.

True, but unless there is big demand for this, then they can just recall yf.set_network between different queries. Need to balance against code complexity, this code has to be maintained. I like set_network because it will reduce the codebase - all those proxy and session arguments elsewhere can be removed (eventually, but first deprecation warnings).

@R5dan
Copy link
Contributor Author

R5dan commented Jan 9, 2025

I could have it both return a typed dict with all of the config, that way it can be stored and passed to yf.set_network but also doesn't add too much code.

What do you think @ValueRaider?

@R5dan
Copy link
Contributor Author

R5dan commented Jan 9, 2025

Also where would you like this to go? I would put it in utils but that would result in a circular import.

@R5dan
Copy link
Contributor Author

R5dan commented Jan 9, 2025

Do you think I should make a new PR with the updated version (get rid of the Merge branch 'dev' into config? Check out my ConfigV2 branch

@R5dan R5dan mentioned this pull request Jan 10, 2025
@R5dan
Copy link
Contributor Author

R5dan commented Jan 10, 2025

Superseded by #2216

@R5dan R5dan closed this Jan 10, 2025
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.

2 participants