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

Use Attribute Name Based on Attribute Class #160

Merged
merged 8 commits into from
Sep 20, 2016
Merged

Use Attribute Name Based on Attribute Class #160

merged 8 commits into from
Sep 20, 2016

Conversation

lita
Copy link

@lita lita commented Sep 18, 2016

If you use attr_name, such that the name in the Dynamo database is different from the ORM, update_item would do the wrong thing and create a new attribute using the ORM name.

This PR fixes that bug.

@coveralls
Copy link

coveralls commented Sep 18, 2016

Coverage Status

Coverage remained the same at 97.5% when pulling 2fbedf9 on lyft:use-attr-name into 5229759 on jlafon:devel.

@lita
Copy link
Author

lita commented Sep 18, 2016

@danielhochman @jmphilli

@coveralls
Copy link

Coverage Status

Coverage decreased (-0.2%) to 97.297% when pulling bc29e99 on lyft:use-attr-name into 5229759 on jlafon:devel.

@coveralls
Copy link

coveralls commented Sep 18, 2016

Coverage Status

Coverage increased (+0.003%) to 97.503% when pulling bc29e99 on lyft:use-attr-name into 5229759 on jlafon:devel.

@coveralls
Copy link

coveralls commented Sep 18, 2016

Coverage Status

Coverage increased (+0.003%) to 97.503% when pulling 16c8876 on lyft:use-attr-name into 5229759 on jlafon:devel.

@coveralls
Copy link

coveralls commented Sep 18, 2016

Coverage Status

Coverage increased (+0.003%) to 97.503% when pulling 1e4c999 on lyft:use-attr-name into 5229759 on jlafon:devel.

@@ -349,12 +349,12 @@ def update_item(self, attribute, value=None, action=None, conditional_operator=N
if len(expected_values):
kwargs.update(expected=self._build_expected_values(expected_values, UPDATE_FILTER_OPERATOR_MAP))
kwargs[pythonic(ATTR_UPDATES)] = {
attribute: {
attribute_cls.attr_name: {
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

i wonder if we should pull the name out up here and save to a local var

Copy link
Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

did you want to save it to a local var for readability?

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

ya just so we don't have to keep accessing it that way.. seems error prone if someone changes this in the future, but may just be a matter of opinion

Copy link
Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

ok

ACTION: action.upper() if action else None,
}
}
if action is not None and action.upper() != DELETE:
kwargs[pythonic(ATTR_UPDATES)][attribute][VALUE] = {ATTR_TYPE_MAP[attribute_cls.attr_type]: value}
kwargs[pythonic(ATTR_UPDATES)][attribute_cls.attr_name][VALUE] = {ATTR_TYPE_MAP[attribute_cls.attr_type]: value}
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

we could then use that var here...

@danielhochman
Copy link
Contributor

+1

@danielhochman danielhochman merged commit 4ab34ca into pynamodb:devel Sep 20, 2016
@danielhochman danielhochman deleted the use-attr-name branch September 20, 2016 18:18
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.

None yet

4 participants