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

Config #2216

Open
wants to merge 21 commits into
base: dev
Choose a base branch
from
Open

Config #2216

wants to merge 21 commits into from

Conversation

R5dan
Copy link
Contributor

@R5dan R5dan commented Jan 10, 2025

Supersedes #2209 with better implementation.

Changes:

  • Deprecates all proxy, session and timeout in functions
  • Adds yf.set_config
  • Allows setting:
    • proxy
    • session
    • timout
    • url
    • language
    • region
  • Adds deprecator for future use
  • Adds docs for use of yf.set_config

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

R5dan commented Jan 11, 2025

@ValueRaider what do you think? Happy to move yf.set_config if you can think of somewhere. I have deprecated all of the session, proxy and timeout (thats why its adding code not removing it but that can all go away in a few versions time)

@R5dan
Copy link
Contributor Author

R5dan commented Jan 19, 2025

@ValueRaider What do you think?

@ValueRaider
Copy link
Collaborator

I haven't had time to try and review yet.

@ValueRaider
Copy link
Collaborator

ValueRaider commented Jan 25, 2025

yf.set_config is fine.

Remove stuff to simplify deprecated:

  • removed_in

  • is **message_params actually used anywhere? If not, remove it.

  • remove since as well, how can developer always know which version their PR will end up in?

User should not be informed on internal deprecations that aren't their fault. Best solution probably is: if proxy argument not None, pass it to YfData, and leave a comment to remove with deprecations.

> dat.balance_sheet
yfinance/scrapers/fundamentals.py:99: DeprecationWarning: Parameter 'proxy' of Financials.get_financials_time_series is deprecated

Set new on everything, so user is told the new function or argument.

What happens if user changes session with set_config after YfData singleton already created?

Reduce timeout default to 10. I know some places did 30, but others did 10.

@R5dan
Copy link
Contributor Author

R5dan commented Jan 26, 2025

  • remove since as well, how can developer always know which version their PR will end up in?

Unless it is near the next release cycle, it is normally pretty obvious, plus it can always be changed - like I'm going to have to do now. I think this should stay as (1) more information never hurt - even if just for devs and (2) it lets the user know when it was deprecated, so if they really wanted to or needed to could revert back to that until they change it. However i will remove it if needed.

  • removed_in

I think this should probably stay as it tells them exactly when to change by.
With this PR I would also like to add a 2 or 3 release gap between all deprecations and removals as I believe earnings in fundamentals has been deprecated since 0.2.41

  • is **message_params actually used anywhere? If not, remove it.

I don't believe it is however think it should stay for future use.

User should not be informed on internal deprecations that aren't their fault

Yes, I agree. I marked the "getters" with deprecate as they weren't the common "private attributes"

dat.balance_sheet
yfinance/scrapers/fundamentals.py:99: DeprecationWarning: Parameter 'proxy' of Financials.get_financials_time_series is deprecated

That shouldn't raise a Deprecation Warning as it doesn't set proxy. Will fix.

Set new on everything, so user is told the new function or argument.

Not possible for proxy session or timeout as they are simply removed. I don't believe I am deprecating anything that have replacements.

What happens if user changes session with set_config after YfData singleton already created?

In my initial testing of it, it seemed to work. However it doesn't seem to set it if its called after. Will have to fix. Any ideas how to?

I will change the default of timeout

@ValueRaider
Copy link
Collaborator

OK keep since. But removed_in is still a problem for us developers - the gap is determined by time more than number of releases and we don't have a regular release schedule.

**message_params should stay for future use

Without a current use case it isn't immediately obvious what it does. I figured it out and saw a bug, it's broken. Also the variable name could be more specific.

Not possible for proxy session or timeout as they are simply removed. I don't believe I am deprecating anything that have replacements.

You need to inform users of the replacement in that deprecation message. That's my point.

Move set_config to the YfData class like a normal setter function.

Run the unit tests to check for bugs. You might need to rebase to latest dev (not merge) to pull in fixes.

@R5dan
Copy link
Contributor Author

R5dan commented Jan 27, 2025

@ValueRaider I have made changes to the branch having already rebased and it now wont let me push due to:

To https://github.com/R5dan/yfinance-package.git
 ! [rejected]        ConfigV2 -> ConfigV2 (non-fast-forward)
error: failed to push some refs to 'https://github.com/R5dan/yfinance-package.git'
hint: Updates were rejected because the tip of your current branch is behind
hint: its remote counterpart. If you want to integrate the remote changes,
hint: use 'git pull' before pushing again.
hint: See the 'Note about fast-forwards' in 'git push --help' for details.

But there isn't any upstream changes etc. Any ideas?

@ValueRaider
Copy link
Collaborator

Immediately after the rebase (done on your local system), do a force push to your fork #1084

@R5dan
Copy link
Contributor Author

R5dan commented Jan 28, 2025

@ValueRaider How is this?

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