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

CLN: Cleanup subclass #110

Merged
merged 1 commit into from Nov 11, 2015

Conversation

Projects
None yet
4 participants
@sinhrks
Member

sinhrks commented Oct 17, 2015

Closes #106. Clean-up most of classes to use common logic.

@femtotrader

This comment has been minimized.

Contributor

femtotrader commented Oct 18, 2015

Your BaseReader should probably have a _get_session method

def _get_session(self, session):
    """Returns a requests.Session if session is None
    or session if it's not None (cached session
    with requests-cache for example)
    :param session:
    :return:
    """
    if session is None:
        session = requests.Session()
    return session

and __init__ should accept a session parameter (default to None)

so we will do

self.session = self._get_session(session)

this will allow to pass a session (with proxy or cache)

@sinhrks sinhrks force-pushed the sinhrks:cln_subclass branch 2 times, most recently from e517194 to 66f8e65 Oct 19, 2015

@sinhrks

This comment has been minimized.

Member

sinhrks commented Oct 19, 2015

OK, updated to add a session, I've chosen a word "init" as it is not for getting.

BTW, found a problem that requests doesn't allow to specify pausing between retry. Thus, current pause kw can't work now... I'll implement retry-pause logic by myself.

@sinhrks

This comment has been minimized.

Member

sinhrks commented Oct 20, 2015

Implemented the logic to handle retry and pause. Now ready for review.

@femtotrader

This comment has been minimized.

Contributor

femtotrader commented Oct 20, 2015

Sorry but I don't see recent commit (authored 11 days ago)

https://github.com/sinhrks/pandas-datareader/tree/cln_subclass

@sinhrks

This comment has been minimized.

Member

sinhrks commented Oct 20, 2015

It's squashed to the single commit, following pandas manners.

@femtotrader

This comment has been minimized.

Contributor

femtotrader commented Oct 20, 2015

Ok yes sorry... I was mistaken by commit date.

+1 about for your cleanup and great work

@bashtage

This comment has been minimized.

Contributor

bashtage commented Oct 22, 2015

Is anything in base for public use? If not private seems more correct.

@sinhrks sinhrks force-pushed the sinhrks:cln_subclass branch from 66f8e65 to 71df260 Oct 23, 2015

@sinhrks

This comment has been minimized.

Member

sinhrks commented Oct 23, 2015

@bashtage No, changed it to private. Rebased and squashed.

@davidastephens

This comment has been minimized.

Member

davidastephens commented Nov 11, 2015

Thanks @sinhrks This looks great.

davidastephens added a commit that referenced this pull request Nov 11, 2015

@davidastephens davidastephens merged commit 0e22ac1 into pydata:master Nov 11, 2015

2 checks passed

continuous-integration/travis-ci/pr The Travis CI build passed
Details
coverage/coveralls Coverage increased (+0.2%) to 95.473%
Details

@sinhrks sinhrks deleted the sinhrks:cln_subclass branch Nov 11, 2015

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment