Rework GithubIntegration #2556
Rework GithubIntegration #2556
Conversation
EnricoMi
force-pushed
the
github-integration-rework-3
branch
from
f998c4e
to
881f560
Compare
| timeout=Consts.DEFAULT_TIMEOUT, | ||
| user_agent=Consts.DEFAULT_USER_AGENT, | ||
| per_page=Consts.DEFAULT_PER_PAGE, | ||
| verify=True, | ||
| retry=None, | ||
| pool_size=None, |
There was a problem hiding this comment.
This is an API breaking change if end users are using positional arguments
There was a problem hiding this comment.
That is right. The rational here is the jwt_* arguments have been added quite recently and should be rarely used. So this breaks for little user code only, and is easy to fix. This will be flagged in the change log.
I favour similarity to github.Github signature here over not breaking this API for everyone.
github/GithubIntegration.pyi
Outdated
There was a problem hiding this comment.
Since this is a, relatively, small class, could we delete this pyi file and move all the types over to the py file since you're already touching this file?
There was a problem hiding this comment.
I'd prefer to deal with that separately.
| :param timeout: integer | ||
| :param user_agent: string | ||
| :param per_page: int | ||
| :param verify: boolean or string |
There was a problem hiding this comment.
Should this document what values of verify there are? I don't know why this would ever be a str, or what values it would have
There was a problem hiding this comment.
Ah, I tracked it down. this eventually flows to requests.session which takes the verify argument and uses it. I understand.
There was a problem hiding this comment.
Yes, this should reference requests.session in the docs, maybe one day in the future.
| self.assertEqual( | ||
| kwargs.keys(), github.Requester.Requester.__init__.__annotations__.keys() | ||
| ) |
EnricoMi
force-pushed
the
github-integration-rework-3
branch
from
881f560
to
494a930
Compare
Both
github.Githubandgithub.GithubIntegrationare main entry points of this library. They create aRequesterinstance that handles all requests to the Github REST API. Requester has numerous arguments likebase_urlortimeout.Firstly,
GithubandGithubIntegrationshould support the same (full) set ofRequesterarguments.Secondly, creating a
Githubinstance for a Github App Installation coming fromGithubIntegrationshould use the sameRequesterarguments (except forauth). It should suffice to definebase_urland other parameters once.Fixes issues like #2455.