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

[2.0] Add copy method to PreparedRequest objects #1476

Merged
merged 1 commit into from Jul 27, 2013

Conversation

Projects
None yet
3 participants
@sigmavirus24
Member

sigmavirus24 commented Jul 21, 2013

I really think it is only needed for PreparedRequest objects, but I could see a desire to implement it on Request objects too.

The reasoning is simple. Currently, we have duplicated code to make copies of PreparedRequests to preserve the history of the requests sent and be able to make new ones. We could not only reduce duplication, but also ensure (via tests) that the library won't break in weird ways because of this in the future. It would be a dead-simple method to write, and would be very useful and probably even more useful to authentication handler authors.

@sigmavirus24

This comment has been minimized.

Show comment
Hide comment
@sigmavirus24
Member

sigmavirus24 commented Jul 20, 2013

For reference, here's one way of implementing this.

@Lukasa

This comment has been minimized.

Show comment
Hide comment
@Lukasa

Lukasa Jul 21, 2013

Member

I think there's no good reason not to go straight to a Pull Request with that commit. =)

Member

Lukasa commented Jul 21, 2013

I think there's no good reason not to go straight to a Pull Request with that commit. =)

@sigmavirus24

This comment has been minimized.

Show comment
Hide comment
@sigmavirus24

sigmavirus24 Jul 21, 2013

Member

Kind of wish I could just turn this issue into a PR but I don't have the privileges here. Also we should make that 2.0 branch so I can target that since you cannot edit a PR's target after it has been created.

Member

sigmavirus24 commented Jul 21, 2013

Kind of wish I could just turn this issue into a PR but I don't have the privileges here. Also we should make that 2.0 branch so I can target that since you cannot edit a PR's target after it has been created.

@sigmavirus24

This comment has been minimized.

Show comment
Hide comment
@sigmavirus24

sigmavirus24 Jul 21, 2013

Member

Actually, @Lukasa, you can do that for me with this issue. :-P

Member

sigmavirus24 commented Jul 21, 2013

Actually, @Lukasa, you can do that for me with this issue. :-P

@Lukasa

This comment has been minimized.

Show comment
Hide comment
@Lukasa

Lukasa Jul 21, 2013

Member

Don't you have the same privileges as me with this repository?

Member

Lukasa commented Jul 21, 2013

Don't you have the same privileges as me with this repository?

@sigmavirus24

This comment has been minimized.

Show comment
Hide comment
@sigmavirus24

sigmavirus24 Jul 21, 2013

Member

As of the last time I checked, no. I'll try again though and edit this comment with the results.

Clearly I was wrong. :)

Member

sigmavirus24 commented Jul 21, 2013

As of the last time I checked, no. I'll try again though and edit this comment with the results.

Clearly I was wrong. :)

@kennethreitz

This comment has been minimized.

Show comment
Hide comment
@kennethreitz

kennethreitz Jul 23, 2013

Member

👍

Member

kennethreitz commented Jul 23, 2013

👍

@sigmavirus24

This comment has been minimized.

Show comment
Hide comment
@sigmavirus24

sigmavirus24 Jul 23, 2013

Member

❤️ 🍔 💇

Member

sigmavirus24 commented Jul 23, 2013

❤️ 🍔 💇

@sigmavirus24

This comment has been minimized.

Show comment
Hide comment
@sigmavirus24

sigmavirus24 Jul 26, 2013

Member

@kennethreitz is this good for merging?

Member

sigmavirus24 commented Jul 26, 2013

@kennethreitz is this good for merging?

@sigmavirus24

This comment has been minimized.

Show comment
Hide comment
@sigmavirus24

sigmavirus24 Jul 26, 2013

Member

Also, do you want tests for this?

Member

sigmavirus24 commented Jul 26, 2013

Also, do you want tests for this?

@kennethreitz

This comment has been minimized.

Show comment
Hide comment
@kennethreitz

kennethreitz Jul 27, 2013

Member

oh thought i did

Member

kennethreitz commented Jul 27, 2013

oh thought i did

kennethreitz added a commit that referenced this pull request Jul 27, 2013

Merge pull request #1476 from sigmavirus24/add_copy_to_prepared_requests
[2.0] Add copy method to PreparedRequest objects

@kennethreitz kennethreitz merged commit 77bd9c4 into requests:master Jul 27, 2013

1 check passed

default The Travis CI build passed
Details
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment