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__ #75

Open
wants to merge 2 commits into
base: master
Choose a base branch
from

Conversation

mattsb42
Copy link

@mattsb42 mattsb42 commented Feb 2, 2020

The main discussion of the problem is in #74, but TLDR is that IncompleteRequest.__getattr__ mutating and returning self makes a lot of use-cases needlessly complicated.

This is a breaking change.

fixes: #74

I also fixed setup.py to actually work with the tox config. It looks like that used to be there but got rolled back as part of an unrelated set of changes.

@mattsb42 mattsb42 requested a review from gene1wood February 2, 2020 23:14
@gene1wood
Copy link
Collaborator

@mattsb42 Thank you for the PR!

How would you propose going about introducing this breaking change (so as to avoid folks getting the new version and it breaking existing codebases?

@mattsb42
Copy link
Author

This is a good question, and IMO really depends on your plans for the project. It looks like there's been a bunch of recent movement here, I'm guessing with Mozilla taking over the project? Maybe #39 would be an opportune time to make some significant changes?

The reason I think this is important is that the current structure makes it very difficult to pass around and use any partial constructions. You have to end up doing silly things like this[1] to avoid continuously messing up the url.

Another possible option for accomplishing this without breaking backwards compatibility would be to let the user pass an option on instantiation that would determine whether or not this should be a mutable or immutable client. That would increase the API complexity, but would let the user choose this behavior if they want it, and leave the default behavior as-is.

[1]
https://github.com/mattsb42/repo-manager/blob/0bef9d6f85e67c54201f89a9a1668dafa8e3d73f/src/repo_manager/_groups/branches.py#L63-L80

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 this pull request may close these issues.

immutable IncompleteRequest.__getattr__
2 participants