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

Allow getting transactions with info in one request. #4

Open
wants to merge 1 commit into
base: master
Choose a base branch
from

Conversation

lichvarm
Copy link

This patch allows to read both transactions and the account information in one request. This is useful to avoid the server request rate limit (error 409).

@honzajavorek
Copy link
Owner

Thank you! However, I'd like to solve this without abandoning straightforward API. I'd like to provide clear and usable interface while abstracting the implementation. Therefore I think the way to go is to cache the first response and then return information and transaction again in separate methods, with consistent API.

One problem with this would be up-to-dateness of returned data. User expects the data he/she gets are realtime, with no cache involved. I'll think about this.

@lichvarm
Copy link
Author

Thanks for looking into it. I'll be happy with any solution that allows me to get both results without hitting the 30 second limit.

I thought about caching, but it seemed to me the account info and the transaction list should go together. They are both relevant to the specified period as the account info includes the closing balance and may include the opening balance. How about breaking the API and always returning the info and the list as a dict or a tuple?

@honzajavorek
Copy link
Owner

I thought about it and I prefer following solution:

  • tune the API little bit so it provides info together with transactions - possibly it could return something less primitive then just lists and dicts (I'd prefer enhanced subclasses of those with ability to provide more information, so the change would be nice and seamless)
  • introduce some 30s caching and handle HTTP 409, so the library returns the same results as in previous request in case 409 occures - this would be the right thing to do, because in that moment there is no way how to get more recent information than the previous one

I see only one problem with this - fast requests to different methods still cannot use caching and 409 will be thrown (I'd prefer custom exception though). At least, info would be still available in every response, lowering the number of needed requests.

@Visgean
Copy link
Contributor

Visgean commented Feb 12, 2015

I am definitely not reloading info but I think that otherwise the api should fall loudly and clearly - with specific error and leave the caching up to the user..

@honzajavorek
Copy link
Owner

Time and @Visgean convinced me that propagation of custom exception is the right thing to do.

The interface is a different beast and there are some good points about balances etc. I could introduce parallel methods returning both or I could tune the existing API as I mentioned above. Let's see.

@Visgean
Copy link
Contributor

Visgean commented Feb 12, 2015

Maybe something like this:

class FioBank(object):
    _info = None

    def info(self):
        if self._info:
            return self._info
        today = date.today()
        data = self._request('periods', from_date=today, to_date=today)
        self._info = self._parse_info(data)
        return _info

But there needs to be some mechanism for reloading/refreshing the info....

@honzajavorek
Copy link
Owner

It's not in my current possibilities to work on it (all my opensource focus now goes to the planned new version of https://github.com/pyvec/python.cz/), but PRs with some neat, non-bc-breaking solutions are welcome :)

@honzajavorek
Copy link
Owner

Thanks for all the work spent on this. In 2024 I think we could add a separate method which returns both info and transactions. That method would be called transactions and would be a preferred way how to use the library. The current methods would bring up deprecation warnings.

@nijel
Copy link
Contributor

nijel commented Oct 25, 2024

I've implemented recommended approach in #38

nijel added a commit to nijel/fiobank that referenced this pull request Nov 14, 2024
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.

4 participants