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

Provide better type checking for values in from_dict #587

Merged
merged 4 commits into from
Oct 8, 2016
Merged
Show file tree
Hide file tree
Changes from 1 commit
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Jump to
Jump to file
Failed to load files.
Diff view
Diff view
18 changes: 15 additions & 3 deletions tests/test_dict.py
Expand Up @@ -5,8 +5,8 @@


class TestDict(unittest.TestCase):
def test_exclusive(self):
d = {
def setUp(self):
self.d = {
"Cpu": 1,
"Environment": [
{
Expand Down Expand Up @@ -34,8 +34,9 @@ def test_exclusive(self):
]
}

def test_exclusive(self):
t = Template()
cd = ecs.ContainerDefinition.from_dict("mycontainer", d)
cd = ecs.ContainerDefinition.from_dict("mycontainer", self.d)
td = ecs.TaskDefinition(
"taskdef",
ContainerDefinitions=[cd],
Expand All @@ -45,5 +46,16 @@ def test_exclusive(self):
t.add_resource(td)
t.to_json()

def test_invalid_toplevel_property(self):
self.d["BlahInvalid"] = "Invalid"
with self.assertRaises(AttributeError):
ecs.ContainerDefinition.from_dict("mycontainer", self.d)

def test_invalid_sub_property(self):
self.d["Environment"][0]["BlahInvalid"] = "Invalid"
with self.assertRaises(AttributeError):
ecs.ContainerDefinition.from_dict("mycontainer", self.d)


if __name__ == '__main__':
unittest.main()
41 changes: 37 additions & 4 deletions troposphere/__init__.py
Expand Up @@ -162,10 +162,43 @@ def validate(self):
pass

@classmethod
def from_dict(cls, title, dict):
obj = cls(title)
obj.properties.update(dict)
return obj
def _from_dict(cls, title=None, **kwargs):
props = {}
for prop_name, value in kwargs.items():
try:
prop_attrs = cls.props[prop_name]
Copy link
Contributor

Choose a reason for hiding this comment

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

cls.props could be missing too

Copy link
Member Author

Choose a reason for hiding this comment

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

I don't think it can - this is on AWSBaseObject, and AWSBaseObject sets up self.props in its init.

Copy link
Contributor

Choose a reason for hiding this comment

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

oh you're right, this is the cls object. i was thinking of the function you had before that took any class

except KeyError:
raise AttributeError("Object type %s does not have a "
"%s property." % (cls.__name__,
prop_name))
prop_type, required = prop_attrs
Copy link
Contributor

Choose a reason for hiding this comment

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

maybe just prop_type = prop_attrs[0] since you don't use required?

Copy link
Member Author

Choose a reason for hiding this comment

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

Good call, updating.

if prop_name in kwargs:
value = kwargs[prop_name]
is_aws_object = False
try:
is_aws_object = issubclass(prop_type, BaseAWSObject)
# prop_type isn't a class
except TypeError:
pass
if is_aws_object:
value = prop_type._from_dict(**kwargs[prop_name])
Copy link
Contributor

Choose a reason for hiding this comment

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

what if kwargs[prop_name] isn't a dict?


if isinstance(prop_type, list):
if not isinstance(kwargs[prop_name], list):
Copy link
Contributor

Choose a reason for hiding this comment

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

this could just be value that you pulled out aboe

raise TypeError("Attribute %s must be a "
"list." % prop_name)
new_value = []
for v in value:
new_value.append(prop_type[0]._from_dict(**v))
Copy link
Contributor

Choose a reason for hiding this comment

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

what if v isn't a dict? it seems like this needs the same check for aws object

value = new_value
props[prop_name] = value
if title:
return cls(title, **props)
return cls(**props)

@classmethod
def from_dict(cls, title, d):
return cls._from_dict(title, **d)

def JSONrepr(self):
for k, (_, required) in self.props.items():
Expand Down