agithub icon indicating copy to clipboard operation
agithub copied to clipboard

immutable IncompleteRequest.__getattr__

Open mattsb42 opened this issue 5 years ago • 0 comments

I've been using this client in one of my projects[1] and I'm loving that I don't need to do anything to incorporate new API additions but I've run into a lot of issues trying to reuse partial constructions because IncompleteRequest.__getattr__ modifies self rather than returning a new request.

For example, if I want to get the branch protection details for every branch in a repo, I would assume that I could just do something along the lines of the below (actual url mutation in comments):

import os
import sys

from agithub.GitHub import GitHub

def all_branch_protection_data(username, repo_name):
    client = GitHub(token=os.environ["GITHUB_TOKEN"], paginate=True)

    repo = getattr(getattr(client.repos, username), repo_name)
    # repo.url == /repos/:owner/:repo
    print(repo.url)

    status, data = repo.branches.get()
    # repo.url == /repos/:owner/:repo/branches
    print(repo.url)

    # BASELINE == /repos/:owner/:repo/branches
    # LOOP_BASELINE = BASELINE
    for branch in data:
        branch = getattr(repo.branches, branch["name"])
        # branch is repo; repo.url == LOOP_BASELINE/branches/:branch
        print(repo.url)
        status, data = branch.protection.get()
        # branch is repo; repo.url == LOOP_BASELINE/branches/:branch/protection
        print(repo.url)
        yield data
        # LOOP_BASELINE = LOOP_BASELINE/branches/:branch/protection

if __name__ == "__main__":
    list(all_branch_protection_data(sys.argv[1], sys.argv[2]))

However, if I were to try this, it would not do what I would think it should, because every call to __getattr__ mutates repo.url.

/repos/mattsb42/rhodes
/repos/mattsb42/rhodes/branches
/repos/mattsb42/rhodes/branches/branches/development
/repos/mattsb42/rhodes/branches/branches/development/protection
/repos/mattsb42/rhodes/branches/branches/development/protection/branches/master
/repos/mattsb42/rhodes/branches/branches/development/protection/branches/master/protection
/repos/mattsb42/rhodes/branches/branches/development/protection/branches/master/protection/branches/wat
/repos/mattsb42/rhodes/branches/branches/development/protection/branches/master/protection/branches/wat/protection

I can work around this by resetting repo.url at the start of every iteration of the loop, but this seems to me like a common enough general use-case that I suspect that a large percentage of users are also running into this.

The fix for this is fairly simple, but would be a potentially significant breaking change to the client behavior. I'll go ahead and send up a PR for this because the change is so small, but I'm also happy to discuss the problem more here if you like.

[1] https://github.com/mattsb42/repo-manager

mattsb42 avatar Feb 02 '20 23:02 mattsb42