Add event #139

kelseymorris95 · 2016-06-16T15:45:59Z

Added an Event class, edited Events.get_event() to return a list of them. Added basic Event unit test file for future use.

Fixes #15.

boxcla · 2016-06-16T15:46:09Z

Hi @kelseymorris95, thanks for the pull request. Before we can merge it, we need you to sign our Contributor License Agreement. You can do so electronically here: http://opensource.box.com/cla

Once you have signed, just add a comment to this pull request saying, "CLA signed". Thanks!

HootyMcOwlface · 2016-06-16T16:01:34Z

boxsdk/object/event.py

+    def get_url(self, *args):
+        """
+        Base class override.
+        Disallow this method for this subclass, should not access Event by event_id.


Minor: Note that there is no get method in the box v2 api for Event.

HootyMcOwlface · 2016-06-16T16:10:44Z

test/unit/object/test_event.py

+
+def test_init_event(mock_box_session):
+    event = Event(mock_box_session, "f82c3ba03e41f7e8a7608363cc6c0390183c3f83", {
+       "type": "event",


formatting here... actually travis is probably complaining about this.
we usually use the format like:
event = Event(
arg1,
next_arg_on_new_line_if_we_are_using_multiline,
{
foo: bar,
baz: wing,
ding: {
but_were_not_super_strict_I_didnt_put_the_above_curley_on_its_own_line: value1,
but_we_are_picky_about_the_last_item_having_a_trailing_comma: value2,
},
},
)

HootyMcOwlface · 2016-06-16T16:31:07Z

test/unit/object/test_events.py

    ]
    events = test_events.generate_events_with_long_polling(**stream_type_kwargs)
-    assert next(events) == mock_event
+    assert next(events) == mock_event_object


instead of having the mock_event_object fixture you could construct the mock Event here based on the mock_event json. Then do the assert check for equality. Then I think you can remove the mock_event fixture.

jmoldow · 2016-06-16T18:28:53Z

boxsdk/object/event.py

+        Base class override.
+        Disallow this method for this subclass, should not access Event by event_id.
+        """
+        raise AttributeError("'Event' class has no method 'get_url'")


Glad that you noticed this issue.

To me, this sort of thing is a code smell. If a baseclass defines a default behavior, and then a subclass is incapable of performing that behavior, chances are, the subclass should not actually inherit from that baseclass.

I believe that to be the case here. BaseObject is a class that is the root for all possible persistent, addressable resource types in the REST API.

Since we've never thought about non-persistent / non-addressable objects before, we didn't think about this when we first created #15.

I have an idea of what we should do here, but I'm curious for your thoughts.

I was thinking the same thing - Event should probably not derive directly from BaseObject.

@kelseymorris95 @jmoldow should we get together to discuss the right plan for this?

Yeah, I had discussed this with Jeremy because I wasn't sure what approach was best. Most methods in BaseObject use get_url(), so I think the only behaviours of BaseObject that we actually want to mimic are init, getitem, repr, object_id, and eq.
Is there an option besides reproducing those behaviors separately in Event?

Check out #140. The first half is unrelated, but the second half overlaps with what we're talking about here.

kelseymorris95 · 2016-06-16T23:58:58Z

CLA signed

boxcla · 2016-06-16T23:58:59Z

Verified that @kelseymorris95 has just signed the CLA. Thanks, and we look forward to your contribution.

jmoldow · 2016-06-17T22:11:02Z

boxsdk/object/api_json_object.py

@@ -0,0 +1,12 @@
+# coding: utf-8
+
+from __future__ import unicode_literals


We should also be using absolute_import in all files.

jmoldow · 2016-06-20T21:40:02Z

boxsdk/object/base_object.py

-    A Box API endpoint for interacting with a Box object.
-    """
+    # Question:
+    # Should this item type be a part of other objects?


No, it isn't necessary. This is just here so that all of its subclasses at least have _item_type defined, even if it is None.

Though this should move to BaseAPIJSONObject, since that is now the base for translation.

jmoldow · 2016-06-20T22:02:54Z

Something to think about: Translator.translate() takes a type, and tries to translate it. If it fails, it'll return BaseObject. Then, the usual procedure is to pass session, object_id, response_object to the class that is returned.

This has two problems:

Ideally, the default return value would be either BaseObject or APIJSONObject, depending on the object. I would choose BaseObject if there were an id, and APIJSONObject otherwise. However, we can't do that since the method only accepts type. We'd need to make a backwards-incompatible change to make it accept additional information.
The types of classes we return have different __init__ parameters. So you can't initialize an object unless you know in advance which type it's going to be. We could fix this by:
- Moving the object creation inside of Translator.translate() (after dealing with the above problem).
- Making APIJSONObject.__init__ have the same argument list as BaseObject.__init__, even though it doesn't need most of them.

Let's not worry about this right now. What we have now works well enough to push what you have as-is. But we'll want to revisit this before or during work on #140.

HootyMcOwlface · 2016-06-21T18:49:10Z

boxsdk/object/api_json_object.py

+from .base_api_json_object import BaseAPIJSONObject, BaseAPIJSONObjectMeta
+
+
+class APIJSONObjectMeta(BaseAPIJSONObjectMeta, ABCMeta):


This is an empty ABCMeta which is a little strange to me. I don't follow the comment about avoiding conflicts. We get BetaAPIJSONObjectMeta thru BaseAPIJSONObject below in APIJSONObject... so, I'm not sure what this is needed for.

Thnx for explaining... I think that's worth a link & comment.
http://code.activestate.com/recipes/204197-solving-the-metaclass-conflict/
or something saying this addresses the known issue with inheriting from 2 classes with separate metaclasses.

HootyMcOwlface · 2016-06-21T19:04:35Z

👍

jmoldow · 2016-06-21T19:40:48Z

boxsdk/object/base_api_json_object.py

+            A JSON object representing the object returned from a Box API request.
        :type response_object:
-            :class:`BoxResponse`
+            :`dict`


No colon here and in other places.

jmoldow · 2016-06-21T22:40:06Z

👍

Thanks @kelseymorris95 ! This all looks great. I'm going to merge it now.

Before releasing this to PyPI, over the next few days, let's make sure we're happy with the names of the new classes, make a final decision about Mapping vs MutableMapping vs dict, and double-check that we aren't introducing any other backwards incompatibilities.

Kelsey Morris added 3 commits June 15, 2016 10:12

Add Event class

016c2f7

Add Event class

64d04f9

Remove print statement

f834121

HootyMcOwlface reviewed Jun 16, 2016
View reviewed changes

Conform to pep8 rules

7f87500

HootyMcOwlface reviewed Jun 16, 2016
View reviewed changes

Add trailing commas, fix constructor formatting

d3dcd71

HootyMcOwlface reviewed Jun 16, 2016
View reviewed changes

Improve Event unit tests

379aa4c

jmoldow reviewed Jun 16, 2016
View reviewed changes

Begin to separate REST behavior from other behavior

6b4f791

kelseymorris95 closed this Jun 16, 2016

kelseymorris95 reopened this Jun 16, 2016

Remove Event class from REST object hierarchy

66ebc91

jmoldow reviewed Jun 17, 2016
View reviewed changes

jmoldow reviewed Jun 20, 2016
View reviewed changes

Kelsey Morris added 2 commits June 21, 2016 09:15

Clean up code, adhere to Python 3 compatibility

4545a7f

Add unit tests for new classes

eb1c903

HootyMcOwlface reviewed Jun 21, 2016
View reviewed changes

jmoldow reviewed Jun 21, 2016
View reviewed changes

Kelsey Morris added 2 commits June 21, 2016 14:32

Improve unit tests

e590817

Fix unit test

c8057a0

jmoldow merged commit 76652d9 into master Jun 21, 2016

Jeff-Meadows deleted the add_event branch June 21, 2017 23:05

jmoldow mentioned this pull request Jul 19, 2017

Auto-translate all Box REST API objects #140

Closed

		@@ -0,0 +1,12 @@
		# coding: utf-8

		from __future__ import unicode_literals

		from .base_api_json_object import BaseAPIJSONObject, BaseAPIJSONObjectMeta


		class APIJSONObjectMeta(BaseAPIJSONObjectMeta, ABCMeta):

Add event #139

Add event #139

Uh oh!

Conversation

kelseymorris95 commented Jun 16, 2016 • edited by jmoldow Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

boxcla commented Jun 16, 2016

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

kelseymorris95 commented Jun 16, 2016

Uh oh!

boxcla commented Jun 16, 2016

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

jmoldow commented Jun 20, 2016

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

HootyMcOwlface commented Jun 21, 2016

Uh oh!

Choose a reason for hiding this comment

Uh oh!

jmoldow commented Jun 21, 2016

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

6 participants

kelseymorris95 commented Jun 16, 2016 •

edited by jmoldow

Loading