-
Notifications
You must be signed in to change notification settings - Fork 1.9k
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
Make resource identifiers immutable #246
Conversation
LGTM |
Creates a read-only property for identifier attributes. | ||
""" | ||
def identifier(self): | ||
return getattr(self, '_' + name, None) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Should we be returning None
if the internal attr doesn't exist? Seems like that would only happen if we had a bug, in which case we'd want it to fail loudly.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Yeah I am fine with that. I do not think that it will happen unless there is a bug. Furthermore, since this is an identifier in general, its value cannot be None
so it make sense not to default to None
.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Scratch what I said, I remembered why I set it a default to None
. It is because when the resource is instantiated there is check to make sure that all of the identifiers are non-None
values: https://github.com/boto/boto3/blob/develop/boto3/resources/base.py#L117. So if I did not use a default, I would get a bad error message like: 'test.Queue' object has no attribute '_url'
instead of the current message of: ValueError: Required parameter url not set
because the identifier getter is called before the ValueError is thrown. I would prefer to keep it as is unless you have any other suggestions.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Yeah that's fine, but let's add a comment here that explains that.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I can do that.
Just had a question about a failure case, otherwise looks good. |
|
Make resource identifiers immutable
Before you could switch out the identifiers on a resource, which can have unintended affects if you switch the identifier and then make subsequent calls with the resource. Now identifiers are read-only, much like how attributes are read-only:
cc @jamesls @mtdowling @rayluo