Move name collision logic to the model #67
Conversation
This change moves and formalizes the name collision logic from the factory
to the resource model. The following has changed:
* Documentation now better matches the factory output.
* Collision renaming order is formalized in one place
* Order: reserved names (meta), load action, identifiers, actions,
subresources, references, collections, and then attributes.
* Renaming resource model attributes/methods now happens at model loading
time rather than class creation time.
The way this works is by creating a mapping of (type, name) tuples to
the renamed value, if it exists. Typically this mapping will be very
sparse and it's fast to create / access. In practice we currently only
have one or two names that collide across all the resource models.
Tests have been updated, some of which needed to define proper Botocore
shapes as the code now looks more closely at those at model load time.
| @@ -182,7 +182,7 @@ def test_resource_references(self): | |||
| self.assertEqual(len(model.references), 1) | |||
|
|
|||
| ref = model.references[0] | |||
| self.assertEqual(ref.name, 'Frob') | |||
| self.assertEqual(ref.name, 'frob') | |||
There was a problem hiding this comment.
This was actually a bug. This is a reference, and as such should have a snake-cased name.
|
Changes Unknown when pulling 1e05404 on model-collision into * on develop*. |
danielgtaylor
added
the
enhancement
| 'type': 'structure', | ||
| 'members': {} | ||
| }) | ||
| shape.members['ETag'] = Shape('ETag', {'type': 'string'}) |
There was a problem hiding this comment.
This is relying on an implementation detail that is subject to change. members is actually a property (a cached property) that happens to create a mutable attr the first time it's retrieved.
Alternatives:
- Pass in a
ShapeResolverwith ashape_map. - Use the botocore.model.DenormalizedStructureBuilder.
|
As someone who hasn't looked at this code in a while, I'm probably missing something, but where are the tests for this feature? Given your description, particularly the second point:
I was looking for a test of exactly like this. A resource that had one or more of those, and we verify they get renamed as X, Y, Z.
|
This fixes a few issues that cropped up during testing and makes the handling of renamed attributes safer with respect to autoloaded resource properties.
| @@ -201,44 +191,8 @@ def _load_waiters(self, attrs, model): | |||
| """ | |||
| for waiter in model.waiters: | |||
| snake_cased = xform_name(waiter.resource_waiter_name) | |||
| snake_cased = self._check_allowed_name( | |||
There was a problem hiding this comment.
I did not see where waiters get checked in the new collision logic.
|
Looks good. The only thing I did not see was the collision handling for waiters. I talked to you about adding it, even though the chance of collisions for waiter_actions is very small especially since |
|
@kyleknap please take another look. I've added waiters to the renaming logic, removed the At this point the model does two things: it loads the JSON data and surfaces it in a Python-friendly way (snake-cased names, and handles renaming so there are no duplicates). Both the factory and docs are a bit simpler now (though I didn't mess much with docs because that will get its own overhaul shortly). |
|
Thanks! 🚢 |
|
|
This change moves and formalizes the name collision logic from the factory
to the resource model. The following has changed:
subresources, references, collections, and then attributes.
time rather than class creation time.
The way this works is by creating a mapping of (type, name) tuples to
the renamed value, if it exists. Typically this mapping will be very
sparse and it's fast to create / access. In practice we currently only
have one or two names that collide across all the resource models.
Tests have been updated, some of which needed to define proper Botocore
shapes as the code now looks more closely at those at model load time.
cc @kyleknap @jamesls