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

Draft
wants to merge 4 commits into
base: dev
Choose a base branch
from
Draft

Add config #2209

wants to merge 4 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

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