Conversation
f998c4e to
881f560
Compare
JLLeitschuh
left a comment
There was a problem hiding this comment.
LGTM. One question about potentially API breaking changes though.
| 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.
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() | ||
| ) |
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.