data type examples #143

freddrake · 2017-05-22T23:48:06Z

introduce Example object for RAML 1.0
use separate test files for 0.8 and 1.0; they will not have anything
in common

codecov-io · 2017-05-23T00:44:12Z

Codecov Report

Merging #143 into v0.2.0-dev will increase coverage by 0.16%.
The diff coverage is 100%.

@@              Coverage Diff               @@
##           v0.2.0-dev     #143      +/-   ##
==============================================
+ Coverage       95.61%   95.78%   +0.16%     
==============================================
  Files              40       42       +2     
  Lines            1940     2018      +78     
==============================================
+ Hits             1855     1933      +78     
  Misses             85       85

Impacted Files	Coverage Δ
ramlfications/models/examples.py	`100% <100%> (ø)`
ramlfications/models/base.py	`100% <100%> (ø)`	⬆️
ramlfications/utils/types.py	`93.18% <100%> (+0.32%)`	⬆️
ramlfications/errors.py	`100% <100%> (ø)`	⬆️
ramlfications/utils/examples.py	`100% <100%> (ø)`
ramlfications/parser/parameters.py	`100% <100%> (ø)`	⬆️

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update 1cc8e26...2861544. Read the comment docs.

econchick · 2017-05-23T16:25:47Z

ramlfications/models/data_types.py

@@ -107,6 +107,23 @@ class RAMLDataType(object):


 @attr.s
+class Example(object):


I think this should be moved into ramlfications/models/parameters.py - the data_types.py module here is meant for actual Data Types; and parameters.py is more for both named parameters and "sub" objects like query/uri params/body, etc

I wonder if there should be a ramlfications.models.examples module for this, then: data types can also have examples, not only parameters.

econchick · 2017-05-23T16:31:44Z

ramlfications/models/data_types.py

+    display_name = attr.ib(default=None)
+
+    # Not sure when validation of examples should get done; leave for now.
+    strict       = attr.ib(default=True)


will need annotation_name too - although they are not implemented yet, if it breaks tests, then either comment it out, or I think it'd work like attr.ib(init=False)

econchick · 2017-05-23T16:32:28Z

ramlfications/models/data_types.py

+
+    """
+    value        = attr.ib()
+    name         = attr.ib(default=None)


I think name should be in repr but no other attribute.

econchick · 2017-05-23T16:48:28Z

ramlfications/parser/parameters.py

+                else:
+                    kwargs.update(parse_examples(raml_version, value))
+            else:
+                kwargs.update(parse_examples(raml_version, value))


Hmm, perhaps this would be cleaner:

if raml_version == "0.8": kwargs["repeat"] = _get(value, "repeat", False) if raml_version == "0.8" and isinstance(value, list): # comment pass else: kwargs.update(parse_examples(raml_version, value))

econchick · 2017-05-23T17:11:23Z

ramlfications/utils/types.py

    return data_type_cls(**data)
+
+
+def parse_examples(raml_version, data):


Similarly with the Example model, I think this would be better in ramlfications/parser/parameters.py

Same response applies here as well, too,

econchick · 2017-05-23T17:31:58Z

tests/integration/data_types/test_examples_08.py

+    # annotations or other facets of an example other than the value.
+    #
+    fp = get_form_param(api, "/with_example_structured")
+    assert fp.example == {"value": "This whole map is a value."}


I would also assert that examples doesn't exist.

econchick · 2017-05-23T17:32:20Z

tests/integration/data_types/test_examples_08.py

+
+def test_base_uri_param(api):
+    p = api.base_uri_params[0]
+    assert p.description.raw == "account name"


I don't think you need to test other parts that are not a part of the example.

econchick · 2017-05-23T17:32:41Z

tests/integration/data_types/test_examples_08.py

@@ -0,0 +1,78 @@
+# -*- coding: utf-8 -*-
+from __future__ import absolute_import, division, print_function
+"""\


New line \ not needed

econchick · 2017-05-23T17:33:01Z

tests/integration/data_types/test_examples_10.py

@@ -0,0 +1,121 @@
+# -*- coding: utf-8 -*-
+from __future__ import absolute_import, division, print_function
+"""\


New line \ not needed

econchick · 2017-05-23T17:33:50Z

tests/integration/data_types/test_examples_10.py

+    assert ex.description.raw == "Send this for a 500"
+    assert ex.display_name.raw == "DON'T DO THIS"
+    assert ex.strict is False
+    assert ex.value == "breakfast"


Can you add a test that example and examples return a runtime error?

econchick

Just a few more nitpicks, but other than that, it looks good!

econchick · 2017-05-23T21:25:18Z

ramlfications/models/data_types.py


    # Not sure when validation of examples should get done; leave for now.
-    strict       = attr.ib(default=True)
+    strict       = attr.ib(default=True, repr=False)


Did you see my comment about annotation_name?

econchick · 2017-05-23T21:26:58Z

ramlfications/utils/types.py

-                raise RuntimeError("example and examples cannot co-exist")
+                # TODO: Emit a lint warning during validation.
+                raise InvalidRAMLStructureError(
+                    "example and examples cannot co-exist")


Nitpick: can you add single quotes around the properties for readability? e.g. "'example' and 'examples' cannot co-exist"

econchick · 2017-05-23T21:27:18Z

ramlfications/utils/types.py

            examples = data["examples"]
            # Must be a map:
            if not isinstance(examples, dict):
                # Need to decide what exception to make this.
-                raise UnknownDataTypeError
+                raise InvalidRAMLStructureError("examples must be a map node")


Same here, add ' around examples

econchick · 2017-05-23T21:30:01Z

ramlfications/utils/types.py

+            if "displayName" in node:
+                data["display_name"] = node["displayName"]
+            if "strict" in node:
+                data["strict"] = node["strict"]


I think with these things (side-stepping the eventual adding of NOTHING or some type of Undefined type), I think it'd be cleaner if it was like this:

data["description"] = BaseContent(node.get("description", "")) data["display_name"] = node.get("displayName", name) data["strict"] = node.get("strict", True)

You'd be able to remove the following 2 lines, too.

econchick · 2017-05-23T21:34:01Z

tests/integration/data_types/test_examples_10.py

+    try:
+        parse_raml(loaded_raml, config)
+    except errors.InvalidRAMLStructureError as e:
+        assert str(e) == 'example and examples cannot co-exist'


The way to assert that an exception has been raised is not with a try/except but with a with pytest.raises:

with pytest.raises(errors.InvalidRAMLStructureError) as e: parse_raml(loaded_raml, config) msg = # error message assert msg in e.value.args[0]

and same with the example below.

econchick

super tiny nit picks! sorry!

econchick · 2017-05-23T23:40:29Z

ramlfications/utils/types.py

-                data["strict"] = node["strict"]
-    if "display_name" not in data:
-        data["display_name"] = name
+    if isinstance(node, dict) and "value" in node:


couldn't you also remove the and "value" in node: and do similarly `data["value"] = node.get("value")? I know there has to be a value, but that can be caught in validation.

Not in this case. When there's a value key, the entire interpretation changes. If there isn't a value key, we already have the value, and we don't want to clobber it. Only when we have the value key can the other bits of metadata be present.

ahh okay 👍

econchick · 2017-05-23T23:40:48Z

tests/integration/data_types/test_examples_10.py

        parse_raml(loaded_raml, config)
-    except errors.InvalidRAMLStructureError as e:
-        assert str(e) == 'example and examples cannot co-exist'
+    assert str(e.value) == 'example and examples cannot co-exist'


double quotes please

econchick · 2017-05-23T23:40:56Z

tests/integration/data_types/test_examples_10.py

        parse_raml(loaded_raml, config)
-    except errors.InvalidRAMLStructureError as e:
-        assert str(e) == 'examples must be a map node'
+    assert str(e.value) == 'examples must be a map node'


same here, double quotes

freddrake · 2017-05-24T00:38:43Z

Ready for review again.

econchick

Just squash commits then I'll merge!

- introduce Example object for RAML 1.0 - use separate test files for 0.8 and 1.0; they will not have anything in common

freddrake · 2017-05-24T16:30:57Z

Commits have been squashed.

sichvoge · 2017-05-25T08:37:19Z

Awesome! :)

econchick added the in progress label May 22, 2017

freddrake requested a review from econchick May 22, 2017 23:48

freddrake force-pushed the fd-data-type-examples branch from 1dbce35 to a4a013d Compare May 23, 2017 05:58

econchick suggested changes May 23, 2017

View reviewed changes

freddrake force-pushed the fd-data-type-examples branch from 1d65e3f to f8ecf24 Compare May 23, 2017 22:23

econchick suggested changes May 23, 2017

View reviewed changes

econchick reviewed May 24, 2017

View reviewed changes

support for data type examples:

a10fec3

- introduce Example object for RAML 1.0 - use separate test files for 0.8 and 1.0; they will not have anything in common

freddrake force-pushed the fd-data-type-examples branch from 2861544 to a10fec3 Compare May 24, 2017 16:17

econchick approved these changes May 24, 2017

View reviewed changes

econchick merged commit 890396e into jdiegodcp:v0.2.0-dev May 24, 2017

econchick removed the in progress label May 24, 2017

freddrake mentioned this pull request May 24, 2017

Double-check support for Example/Examples #132

Open

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

data type examples #143

data type examples #143

freddrake commented May 22, 2017

codecov-io commented May 23, 2017 •

edited

econchick May 23, 2017

freddrake May 23, 2017

econchick May 23, 2017

econchick May 23, 2017

econchick May 23, 2017

econchick May 23, 2017

freddrake May 23, 2017

econchick May 23, 2017

econchick May 23, 2017

econchick May 23, 2017

econchick May 23, 2017

econchick May 23, 2017

econchick left a comment

econchick May 23, 2017

econchick May 23, 2017

econchick May 23, 2017

econchick May 23, 2017

econchick May 23, 2017

econchick May 23, 2017

econchick left a comment

econchick May 23, 2017

freddrake May 23, 2017

econchick May 24, 2017

econchick May 23, 2017

econchick May 23, 2017

freddrake commented May 24, 2017

econchick left a comment

freddrake commented May 24, 2017

sichvoge commented May 25, 2017

		@@ -107,6 +107,23 @@ class RAMLDataType(object):


		@attr.s
		class Example(object):

		return data_type_cls(**data)


		def parse_examples(raml_version, data):

data type examples #143

data type examples #143

Conversation

freddrake commented May 22, 2017

codecov-io commented May 23, 2017 • edited

Codecov Report

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

econchick left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

econchick left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

freddrake commented May 24, 2017

econchick left a comment

Choose a reason for hiding this comment

freddrake commented May 24, 2017

sichvoge commented May 25, 2017

codecov-io commented May 23, 2017 •

edited