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

Remove elements from a ListAttribute using a list of indexes #754

Merged
merged 9 commits into from Mar 30, 2020

Conversation

dotpmrcunha
Copy link
Contributor

Based on the AWS documents https://docs.aws.amazon.com/amazondynamodb/latest/developerguide/Expressions.UpdateExpressions.html#Expressions.UpdateExpressions.REMOVE

It is possible to delete items from a list inside a ListAttribute using:

--update-expression "REMOVE RelatedItems[1], RelatedItems[2]"

This way I extended the remove in the ListAttribute and added the indexes argument, if there is a valid integer List it will remove the selected elements.

Usage:

list_attribute.remove(indexes=[0, 10])

@dotpmrcunha dotpmrcunha requested review from jpinner-lyft and garrettheel and removed request for jpinner-lyft February 13, 2020 17:06
@garrettheel
Copy link
Member

This looks good overall! Would you mind adding some docs to docs/updates.rst with a usage example? We should be good to go after that.

@dotpmrcunha
Copy link
Contributor Author

Hi, added the document and the example. Thanks

@garrettheel
Copy link
Member

Thanks @dotpmrcunha. I fixed the travis build reporting and it now shows a type error. I'd suggest changing the type signature of Attribute.remove to be compatible

@@ -961,6 +962,13 @@ def __init__(self, hash_key=False, range_key=False, null=None, default=None, att
raise ValueError("'of' must be subclass of MapAttribute")
self.element_type = of

def remove(self, indexes=None):
Copy link
Member

Choose a reason for hiding this comment

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

Apologies for not suggesting earlier, but having thought about this a bit it's probably better to have a separate remove_indexes function on ListAttribute rather than overloading the existing remove. What do you think?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Well... while adding the indexes attribute, I was also thinking in same, separate the methods. I will do that then, it makes more sense.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

And also, instead of using:

class ListAttribute:

    def remove_indexes(self, indexes):
        ....

Thread.notes.remove_indexes([1,2,...])

I add like this:

class ListAttribute:

    def remove_indexes(self, *indexes):
        ....

Thread.notes.remove_indexes(1, 2, ...)

What do you think?

Copy link
Member

Choose a reason for hiding this comment

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

Makes sense to use varargs 👍

docs/updates.rst Outdated
@@ -44,4 +44,5 @@ Any value provided will be serialized using the serializer defined for that attr
`attr_or_value_1` \- `attr_or_value_2`, `attr_or_value_1` \- `attr_or_value_2`, 5 - Thread.views
"list_append( `attr` , `value` )", append( `value` ), Thread.notes.append(['my last note'])
"list_append( `value` , `attr` )", prepend( `value` ), Thread.notes.prepend(['my first note'])
"REMOVE list[index1], list[index1]", "remove_indexes(`index1`, `index2`)", "Thread.notes.remove_indexes(0, 1)"
Copy link
Member

Choose a reason for hiding this comment

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

Suggested change
"REMOVE list[index1], list[index1]", "remove_indexes(`index1`, `index2`)", "Thread.notes.remove_indexes(0, 1)"
"REMOVE list[index1], list[index2]", "remove_indexes(`index1`, `index2`)", "Thread.notes.remove_indexes(0, 1)"

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Fixed

pynamodb/attributes.py Outdated Show resolved Hide resolved
Copy link
Member

@garrettheel garrettheel left a comment

Choose a reason for hiding this comment

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

Thanks for making those changes. Great work!

@garrettheel garrettheel merged commit a5f2369 into pynamodb:master Mar 30, 2020
jpinner-lyft added a commit that referenced this pull request Sep 10, 2020
jpinner-lyft added a commit that referenced this pull request Sep 10, 2020
#838)

This reverts commit a5f2369.

Removal of list elements is already supported using the path expression syntax.
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

2 participants