Fix bugs identified in #72, #74, and #75 #73
Conversation
|
Running more testing, I realized that the lock acquisition call was failing on 2.7 (#74). This also fixes that. |
|
note: The Travis CI tests will fail until #71 is merged |
…e: if the lock was not acquired, this would hit an error
… single command: change the "docs" build comamnd to run the html build and the link check in separate commands
|
rebased following push of #71 |
…from set to list to fix #75
mattsb42-aws
changed the title
| @@ -304,7 +304,7 @@ class TableInfo(object): | |||
| default=None | |||
| ) | |||
| _secondary_indexes = attr.ib( | |||
| validator=attr.validators.optional(iterable_validator(set, TableIndex)), | |||
| validator=attr.validators.optional(iterable_validator(list, TableIndex)), | |||
There was a problem hiding this comment.
What about the type annotation for secondary_indexes on L#341? Did that intentionally not change?
There was a problem hiding this comment.
nope, that was an oversight
CHANGELOG.rst
Outdated
| @@ -2,12 +2,30 @@ | |||
| Changelog | |||
| ********* | |||
|
|
|||
| 1.0.4 -- 2018-05-xx | |||
There was a problem hiding this comment.
It's not ideal, but with the travel and everything in play, if you want to put a reasonable date in here I can commit to getting it pushed by then, depending on your thoughts on the rest of the comments.
| @@ -378,10 +378,10 @@ def refresh_indexed_attributes(self, client): | |||
| table = client.describe_table(TableName=self.name)['Table'] | |||
| self._primary_index = TableIndex.from_key_schema(table['KeySchema']) | |||
|
|
|||
| self._secondary_indexes = set() | |||
| self._secondary_indexes = [] | |||
There was a problem hiding this comment.
Is there anything that relied on the unordered property of set / anything that will be surprised by order and/or (error-case) duplication being introduced?
I didn't see anything in my quick look but you know the code base best. :)
There was a problem hiding this comment.
No, we're actually not doing anything with it at all at the moment, it's just being opportunistically collected since it's all in the same API call. I went with the change to list rather than tuple to retain the mutability and the change away from set rather than adding hash capability to TableIndex because I think the later is something that, while it might be useful later, needs more thought if we want to do.
Issue #, if available: #72
Description of changes:
Working through mypy integration, I ran across #72. This change fixes that bug and adds a test to cover it.
By submitting this pull request, I confirm that my contribution is made under the terms of the Apache 2.0 license.