Adding documentation and object to 'create' function. #56

khardsonhurley · 2017-06-22T22:57:26Z

Removed 'foo_id' and replaced with foo:{} object.
Updated documentation.

- Removed 'foo_id' and replaced with foo:{} object. - Updated documentation.

gmjosack · 2017-06-27T23:30:52Z

README.rst

+
+::
+
+    import pygerduty


This should be import pygerduty.v2 or the module won't be available.

got it, will fix.

gmjosack · 2017-06-27T23:31:23Z

pygerduty/v2.py

@@ -57,6 +57,7 @@ def __init__(self, pagerduty, base_container=None):

        self.pagerduty = pagerduty
        self.base_container = base_container
+        self.type = None


Is this used anywhere?

I think I added it and was going to use it but ended up not using it. I will remove.

gmjosack · 2017-06-28T05:24:47Z

pygerduty/v2.py

+                    'id': kwarg_value,
+                    'type': new_key
+                }
+            elif kwarg_key.endswith('_ids'):


I believe if it ends in _ids it will be a list of ids ["ID1", "ID2"] that needs to be translated to a list of objects

[{"type": "sometype", "id": "ID1"}, ...]

This seems to be doing the same thing as _id

gmjosack · 2017-06-28T15:18:04Z

pygerduty/v2.py

+                    'type': new_key
+                }
+            elif kwarg_key.endswith('_ids'):
+                new_key = kwarg_key[:-3]


I believe we need to go from foo_ids to foos. This will go from foo_ids to foo_. Additionally we should be able to reuse some of this logic for ids from id. Would you mind moving this two functions id_to_obj and ids_to_objs, the latter making use of the former, and adding tests to verify the translations?

- Added two tests to test this functionality.

khardsonhurley · 2017-06-28T19:37:39Z

pygerduty/v2.py

            else:
                new_kwargs[kwarg_key] = kwarg_value
        data[self.sname] = new_kwargs

        response = self.pagerduty.request("POST", path, data=_json_dumper(data), extra_headers=extra_headers)
        return self.container(self, **response.get(self.sname, {}))

+    @staticmethod
+    def id_to_obj(key, value):
+        new_key = key[:-3]


@gmjosack I struggled with figuring out where to put the string slice. Originally I left it inside of the create function but found that hard to test. Putting it in both id_to_obj and ids_to_objs it caused the key to be sliced twice since ids_to_objs depends on id_to_obj. It created something like this:

[{'id': 'PF9KMXH', 'type': 'servi'}, {'id': 'PIJ90N7', 'type': 'servi'}]

My solution was to only do it in id_to_obj, but then I realized we check for '_ids' two times (here and in create function) and this did not see very dry. Is there a better way to do this?

One option is to only pass values to these functions and worry about renaming the key in the outer function that I suggested.

gmjosack · 2017-06-29T05:47:35Z

pygerduty/v2.py

+
+        for kwarg_key, kwarg_value in kwargs.iteritems():
+            if kwarg_key.endswith('_id'):
+                new_kwargs = Collection.id_to_obj(kwarg_key, kwarg_value)


Looks like you're overwriting new_kwargs here? Can you move this entire loop into a function and add a test for it to make sure we're not losing kwargs? The test should have kwargs that are and aren't transformed and test both id and ids types.

gmjosack · 2017-06-29T05:49:17Z

pygerduty/v2.py

+    def id_to_obj(key, value):
+        new_key = key[:-3]
+        if key.endswith('_ids'):
+            new_key = key[:-4] + "s"


Are there any resources that end in y that might be ies instead of s? e.g. policy_ids -> policies. I believe we have a pluralize function that could be reused?

I do not see anywhere where policy_ids are used in the API reference. The v1 reference actually shows:

"escalation_rules": [ { "id": "PYO9B13", "escalation_delay_in_minutes": 22, ... } }

compared to in v2:

{ "escalation_policies": [ { "id": "PANZZEQ", "type": "escalation_policy", ... } }

gmjosack · 2017-06-29T05:50:19Z

pygerduty/v2.py

            else:
                new_kwargs[kwarg_key] = kwarg_value
        data[self.sname] = new_kwargs

        response = self.pagerduty.request("POST", path, data=_json_dumper(data), extra_headers=extra_headers)
        return self.container(self, **response.get(self.sname, {}))

+    @staticmethod
+    def id_to_obj(key, value):
+        new_key = key[:-3]


One option is to only pass values to these functions and worry about renaming the key in the outer function that I suggested.

gmjosack · 2017-06-29T05:50:50Z

tests/collection_test.py

+
+def test_id_to_obj():
+
+	kwarg = {


Are these tabs for indents?

I have been having issues with my tabs, sometimes it does tabs = four spaces and other times it does not. I will check on it.

gmjosack · 2017-06-29T05:53:48Z

tests/collection_test.py

+        },
+        {
+            "id": "PIJ90N7",
+            "type": "services"


The type should be service here I believe. It's the key to this list that needs to be plural that's passed to the api.

@gmjosack Actually I just looked back and this is the reference thing we talked about before:

"services": [ { "id": "PIJ90N7", "type": "service_reference" } ],

You said that you think it won't mind right? I will go ahead and change it to service if you think that will work.

Yeah, pretty sure it happily takes either service or service_reference but I believe it has to be singular.

- First one processes the kwargs and ensures that no kwargs are lost. - Second one cuts off the suffix _id or _ids so that this method is testable.

gmjosack · 2017-07-05T18:34:44Z

tests/collection_test.py

+        "end_time": "2012-06-16T14:00:00-04:00Z",
+        "description": "Description goes here",
+        "service_ids": [
+            "PF9KMXH"


Can you include more than one ID here to ensure it works with multiple?

gmjosack · 2017-07-05T18:34:48Z

tests/collection_test.py

+
+    new_kwargs = pygerduty.v2.Collection.process_kwargs(kwargs_1)
+
+    print new_kwargs


Can you remove this print?

gmjosack · 2017-07-05T18:35:15Z

tests/collection_test.py

+
+def test_process_kwargs():
+
+    kwargs_1 = {


Can you add a singular _id test in here as well to make sure everything works together?

- Added a singular _id test to ensure that the library can process _id into an object. - Added another id to the list of service_ids to ensure it works with multiple id inputs. - Cleaned up code.

Krishelle Hardson-Hurley added 5 commits June 22, 2017 15:53

Adding documentation and object to create request.

f558da3

- Removed 'foo_id' and replaced with foo:{} object. - Updated documentation.

Fixing format in README

ded0b9e

Removing tabs

d6c6b76

Remove type definition

c676039

Add new line to clear Travis complaint.

36d6fe5

gmjosack suggested changes Jun 27, 2017

View reviewed changes

Krishelle Hardson-Hurley added 3 commits June 27, 2017 16:59

Remove type and add conditional for foo_ids.

21e0daf

Update import statement

86e2a19

Fix string formatting

025320a

gmjosack reviewed Jun 28, 2017

View reviewed changes

Krishelle Hardson-Hurley added 2 commits June 27, 2017 22:59

Fix foo_ids to accomodate a list instead of a dictionary.

b9112ab

Removed reference from string formatting, will test manually.

fba343e

gmjosack reviewed Jun 28, 2017

View reviewed changes

Krishelle Hardson-Hurley added 2 commits June 28, 2017 12:22

Add static methods for wrapper around foo_id and foo_ids.

52916d8

- Added two tests to test this functionality.

Call the static methods on the class to clear Travis.

6de014c

khardsonhurley commented Jun 28, 2017

View reviewed changes

gmjosack suggested changes Jun 29, 2017

View reviewed changes

Krishelle Hardson-Hurley added 2 commits June 30, 2017 16:48

Created two static methods for wrapped entities.

767480b

- First one processes the kwargs and ensures that no kwargs are lost. - Second one cuts off the suffix _id or _ids so that this method is testable.

Clearing travis complaint by calling method on the class.

4454fdf

gmjosack reviewed Jul 5, 2017

View reviewed changes

Responding to comments

c0d2fe2

- Added a singular _id test to ensure that the library can process _id into an object. - Added another id to the list of service_ids to ensure it works with multiple id inputs. - Cleaned up code.

gmjosack approved these changes Jul 5, 2017

View reviewed changes

gmjosack merged commit 92fb1b2 into dropbox:master Jul 5, 2017

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Adding documentation and object to 'create' function. #56

Adding documentation and object to 'create' function. #56

khardsonhurley commented Jun 22, 2017

gmjosack Jun 27, 2017

khardsonhurley Jun 27, 2017

gmjosack Jun 27, 2017

khardsonhurley Jun 27, 2017

gmjosack Jun 28, 2017

gmjosack Jun 28, 2017

khardsonhurley Jun 28, 2017

gmjosack Jun 29, 2017

gmjosack Jun 29, 2017

gmjosack Jun 29, 2017

khardsonhurley Jun 30, 2017 •

edited

gmjosack Jun 29, 2017

gmjosack Jun 29, 2017

khardsonhurley Jun 29, 2017

gmjosack Jun 29, 2017

khardsonhurley Jun 29, 2017

gmjosack Jun 29, 2017

gmjosack Jul 5, 2017

gmjosack Jul 5, 2017

gmjosack Jul 5, 2017


		new_kwargs = pygerduty.v2.Collection.process_kwargs(kwargs_1)

		print new_kwargs

Adding documentation and object to 'create' function. #56

Adding documentation and object to 'create' function. #56

Conversation

khardsonhurley commented Jun 22, 2017

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

khardsonhurley Jun 30, 2017 • edited

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

khardsonhurley Jun 30, 2017 •

edited