Quick solution to issue 1874. #1893

Open
wants to merge 1 commit into
from

Conversation

Projects
None yet
3 participants
@msilvey

msilvey commented Dec 5, 2013

Make AssociateAddress return an object with the Association ID instead of a boolean status.

I'm not sure how intrusive a change this would be for the regular/supported use cases of Boto. Anyone who depends on the boolean response will need to change their expected return values.

Matthew Silvey
Quick solution to issue 1874. Make AssociateAddress return an object …
…with the Association ID instead of a boolean status.

@ghost ghost assigned danielgtaylor Dec 6, 2013

@danielgtaylor

This comment has been minimized.

Show comment Hide comment
@danielgtaylor

danielgtaylor Dec 6, 2013

Owner

Unfortunately this is not a backwards-compatible change. Since we don't want to break existing users I think a better approach would be to copy this method, apply your change and give it a new name. It means that we now have two methods that do the same exact call, which is kind of ugly, but it means the code remains backwards compatible and people who want the feature can opt-in by calling the new method.

I might suggest a name like associate_address_object or associate_address_get_object for the new method. @toastdriven, any thoughts?

Edit: I'll also mention that we need to add a unit test to make sure the correct object with the correct fields is returned.

Owner

danielgtaylor commented Dec 6, 2013

Unfortunately this is not a backwards-compatible change. Since we don't want to break existing users I think a better approach would be to copy this method, apply your change and give it a new name. It means that we now have two methods that do the same exact call, which is kind of ugly, but it means the code remains backwards compatible and people who want the feature can opt-in by calling the new method.

I might suggest a name like associate_address_object or associate_address_get_object for the new method. @toastdriven, any thoughts?

Edit: I'll also mention that we need to add a unit test to make sure the correct object with the correct fields is returned.

@toastdriven

This comment has been minimized.

Show comment Hide comment
@toastdriven

toastdriven Dec 7, 2013

Contributor

Yeah, unfortunately, there are users who are depending on this. We have precedent for creating new methods to patch over behavior (see #1572), so I'd be in favor of associate_address_object.

Contributor

toastdriven commented Dec 7, 2013

Yeah, unfortunately, there are users who are depending on this. We have precedent for creating new methods to patch over behavior (see #1572), so I'd be in favor of associate_address_object.

@msilvey

This comment has been minimized.

Show comment Hide comment
@msilvey

msilvey Dec 11, 2013

OK, I'll create an object version and unit testing for it.

msilvey commented Dec 11, 2013

OK, I'll create an object version and unit testing for it.

danielgtaylor added a commit to danielgtaylor/boto that referenced this pull request Jan 3, 2014

danielgtaylor added a commit that referenced this pull request Jan 7, 2014

Merge pull request #1967 from danielgtaylor/ec2-address
Add EC2 associate_address_object method. Fixes #1874, #1893, #1967.
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment