Skip to content

Add rate limiting features #78

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

Open
wants to merge 2 commits into
base: master
Choose a base branch
from
Open

Conversation

anupamsr
Copy link

Add support for session and cache parameters

mftool.py Outdated
@@ -50,6 +50,9 @@ def __init__(self):
self._user_agent = self._const['user_agent']
self._codes = self._const['codes']
self._scheme_codes = self.get_scheme_codes().keys()
self._session = session
Copy link
Owner

Choose a reason for hiding this comment

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

If the session is None then this assignment would be directly contradictory to line 36

@@ -32,7 +32,7 @@ class Mftool:
class which implements all the functionality for
Mutual Funds in India
"""
def __init__(self):
def __init__(self, session=None, cache=None):
Copy link
Owner

Choose a reason for hiding this comment

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

Why do we require to pass a session and cache while creating an object if not None?
I guess it can be handled internally.

@anupamsr
Copy link
Author

anupamsr commented Jun 26, 2024

session=None is how it is handled in yfinance. We can handle it internally, but I would say that these are not handled internally even in yfinance either, so remain similar to it.
image

In fact, looking at the code I see the cache=None is also handled explicitly in yfinance, so I will remove the unecessary if there.
image

@NayakwadiS
Copy link
Owner

NayakwadiS commented Jun 26, 2024

First of all let me know Why do you require a session and cache?

If you are using this library in any personal project then these changes would be part of that project only or we can directly use yfinance instead of mftool.

@anupamsr
Copy link
Author

anupamsr commented Jun 27, 2024

I am using it in my personal capacity already, of course, since I want to rate limit aggresively and I just think it is a good design to re-use existing framework and be compatiable with existing APIs. Hence the PR :) Ultimately your call... I personally don't see any technical issue.

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