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

immutable IncompleteRequest.__getattr__ #74

Open
mattsb42 opened this issue Feb 2, 2020 · 0 comments · May be fixed by #75
Open

immutable IncompleteRequest.__getattr__ #74

mattsb42 opened this issue Feb 2, 2020 · 0 comments · May be fixed by #75

Comments

@mattsb42
Copy link

mattsb42 commented Feb 2, 2020

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 mattsb42 linked a pull request Feb 2, 2020 that will close this issue
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 a pull request may close this issue.

1 participant