[WIP] Return additional request information #37

hkraal · 2016-07-26T14:29:30Z

It's a work in progress but I think a valid solution for retrieving extra data from the CREST request. It can easily be extended with additional fields. This PR is intended to take care of #7 and #18

I will be adding the API version header we get from CCP so the customer knows which version the output is.

I'm looking if I can find a way to prevent breaking backwards compatibility. Any input is appreciated

>>> from pycrest import EVE
>>> eve = EVE()
>>> print(eve())
{'result': {'systems': {'href': 'https://crest-tq.eveonline.com/solarsystems/'}, 'marketPrices': {'href': 'https://crest-tq.eveonline.com/market/prices/'}, 'opportunities': {'groups': {'href': 'https://crest-tq.eveonline.com/opportunities/groups/'}, 'tasks': {'href': 'https://crest-tq.eveonline.com/opportunities/tasks/'}}, 'marketTypes': {'href': 'https://crest-tq.eveonline.com/market/types/'}, 'constellations': {'href': 'https://crest-tq.eveonline.com/constellations/'}, 'serverName': 'TRANQUILITY', 'userCount_str': '17252', 'sovereignty': {'structures': {'href': 'https://crest-tq.eveonline.com/sovereignty/structures/'}, 'campaigns': {'href': 'https://crest-tq.eveonline.com/sovereignty/campaigns/'}}, 'races': {'href': 'https://crest-tq.eveonline.com/races/'}, 'itemTypes': {'href': 'https://crest-tq.eveonline.com/inventory/types/'}, 'wars': {'href': 'https://crest-tq.eveonline.com/wars/'}, 'itemGroups': {'href': 'https://crest-tq.eveonline.com/inventory/groups/'}, 'dogma': {'effects': {'href': 'https://crest-tq.eveonline.com/dogma/effects/'}, 'attributes': {'href': 'https://crest-tq.eveonline.com/dogma/attributes/'}}, 'industry': {'facilities': {'href': 'https://crest-tq.eveonline.com/industry/facilities/'}, 'systems': {'href': 'https://crest-tq.eveonline.com/industry/systems/'}}, 'itemCategories': {'href': 'https://crest-tq.eveonline.com/inventory/categories/'}, 'insurancePrices': {'href': 'https://crest-tq.eveonline.com/insuranceprices/'}, 'npcCorporations': {'href': 'https://crest-tq.eveonline.com/corporations/npccorps/'}, 'alliances': {'href': 'https://crest-tq.eveonline.com/alliances/'}, 'decode': {'href': 'https://crest-tq.eveonline.com/decode/'}, 'virtualGoodStore': {'href': 'https://vgs-tq.eveonline.com/'}, 'authEndpoint': {'href': 'https://login-tq.eveonline.com/oauth/token/'}, 'regions': {'href': 'https://crest-tq.eveonline.com/regions/'}, 'incursions': {'href': 'https://crest-tq.eveonline.com/incursions/'}, 'tournaments': {'href': 'https://crest-tq.eveonline.com/tournaments/'}, 'userCount': 17252, 'serverVersion': 'EVE-TRANQUILITY 14.06.1058053.1058052', 'bloodlines': {'href': 'https://crest-tq.eveonline.com/bloodlines/'}, 'corporations': {'href': 'https://crest-tq.eveonline.com/corporations/'}, 'serviceStatus': 'online', 'time': {'href': 'https://crest-tq.eveonline.com/time/'}, 'marketGroups': {'href': 'https://crest-tq.eveonline.com/market/groups/'}}, 'status_code': 200, 'expires': 20, 'timestamp': 1469542514.0203292}
>>> print(eve().time())
{'result': {'time': '2016-07-26T14:15:40'}, 'status_code': 200, 'expires': 10, 'timestamp': 1469542504.050126}
>>> print(eve().time().result)
{'time': '2016-07-26T14:15:40'}
>>> print(eve().time().expires)
10
>>> print(eve().time().status_code)
200

jonobrien · 2016-07-26T19:30:16Z

So you wrap the response in another dict for result?

Does this mess with dynamically calling an endpoint at all?

wtfrank · 2016-07-26T19:43:44Z

Ok so basically you're overloading getattr to return the extra info we need to be able to get from the cache. What if CCP created link within the CREST namespace that was called "result" (or endpoint_version, status_code, expires_in, expires_at)? There's a potential namespace clash issue (stemming from the design of having "." refer to parts of the crest namespace). Basically the way the APIConnection objects use ".", it makes it difficult to add functions to the object.

I'm trying to think if there's another operator we could use to traverse crest instead of using "." It would look understandable if we used ">" (ge) so we've have urls like eve() > marketData > ... however > is a binary operator so that wouldn't work.

wtfrank · 2016-07-26T19:57:27Z

One option could be to also implement getitem and have only crest stuff accessible by there, so that if crest defined blah and we had also defined a function blah() then calling APIObject.blah would give our function, but APIObject['blah'] would give the crest result. Thats not really ideal though.

hkraal · 2016-07-27T07:46:48Z

@jonobrien I've intended not to and I haven't seen any issues with it so far.

The funny thing here is that we do not clash with our namespaces at all. It took me quite some time to get it working but the unittest gives me the values I'd expect, even if they are in the "same" namespace. I've added a unittest to demonstrate.

As far as I see it we're only missing backwards compatibility (adding .result to contain the data) but this will introduce namespace clashes if I'm not mistaken (haven't gotten that far just yet). If we would like to offer that I would suggest we make it an configurable option or something so the user has a choice.

jonobrien · 2016-07-27T13:44:10Z

If we are being concerned about clashes with possibilities we could go on for days about possibilities that CCP could add. I wouldn't change the dot operator, that sounds like a huge breaking change.

hkraal · 2016-07-27T13:49:51Z

I agree, for now that just asking to much. I do feel we should take it into account in the future but that should be more of a PyCrest 2.0 thing perhaps?

wtfrank · 2016-07-27T13:56:03Z

What about if we return the "meta" attributes such as cache time etc via getitem, and leave getattr for the crest namespace.

That would mean you could do eve().time()['expires'], then we could add any information we can think of without worrying whether they clash with something CCP have added or might add in the future.

jonobrien · 2016-07-27T13:56:59Z

I feel like this package is just supposed to handle interacting with CREST, it seems to have/will have a pretty stable set of functions to handle those different use cases. What else would we really need to add to make this a feature-complete CREST api interacting package?

we have:

public
authed
get/post/etc
authed refresh

we need:

maybe some docs/usage examples
versioning -- soon (tm)
proper cache logic for specific endpoints

jonobrien · 2016-07-27T14:02:36Z

@wtfrank sounds good as long as we document old and new, at least for now, as people would still be using the old via pypy

hkraal · 2016-07-27T14:10:33Z

@jonobrien https://github.com/pycrest/PyCrest/milestone/1, if you have input which isn't there and you think it should gitter or raising an issue would be the places to continue that

jonobrien · 2016-07-27T14:21:22Z

@hkraal Oh I missed the milestone, nevermind then. 👍

wtfrank · 2016-07-27T14:24:37Z

@jonobrien I think the suggestion wouldn't change any of the old way of doing things, as these attributes didn't exist on APIObject in the past!

hkraal · 2016-07-27T14:28:39Z

About @wtfrank 's __getitem__: It would certainly be viable option for me to use that so we don't mingle in the crest namespace. At this point I've no clear view on how we would differentiate crest vs meta attributes but I will take a look at it soon (tm)

wtfrank · 2016-07-27T14:32:37Z

I would say cache returns a tuple: (actual_response, dict_of_meta_info)

Then instead of storing 'result' in the APIObject's _dict attribute, you would store the meta info in APIObject's _meta attribute, then you have the actual data and the metadata separated cleanly.

wtfrank · 2016-07-27T14:37:38Z

So the possibly silly case that I'm worried about: lets say in the future CCP returns expires: {'href' : 'xxx'} in their JSON, you would do eve().expires() to find out whatever data CCP was returning (as current behaviour), you would do eve().expires()['expires'] to find out the expiry of the cached data from the expires crest section, or if you wanted to find the expiry of the root data from the eve object, you would do eve()['expires'].

hkraal · 2016-07-27T14:47:24Z

As far as I'm aware there is only 1 edge case with my code:
Before these changes eve().expires would give you the url self._endpoint + '/expires', with my changed eve().expires would give you the expires metadata property of the eve() request.
If you would like to check the metadata of the expires endpoint you could do eve().expires().expires and it would return you the expires metadata property.

You can see it actually working in the 10918f6 unittest

wtfrank · 2016-07-27T15:17:33Z

Does it work if the JSON returned from eve() contained a key called 'expires' that was like
serverName in the code section I pasted below? So rather than a wrapped dictionary that pycrest would "browse" through, it was data that pycrest would return to you directly.

{'constellations': {href: 'xxx'}, 'serverName': 'TRANQUILITY'}

>>> print eve().serverName, type(eve().serverName)
TRANQUILITY <type 'unicode'>

hkraal · 2016-07-27T15:29:38Z

It does (or I must be misunderstanding you). Let's take an example for clarity:

Let the body being returned when requesting "/market/prices/" is as follows:

{
    "result": "10213",
    "items": [],
    "status_code": 500,
    "pageCount_str": "1",
    "totalCount": 10213
}

This would be the result:

>>> res = self.api().marketData()
>>> res.status_code # HTTP request response code
200
>>> res.result.status_code # body status_code value
500
>>> res.result.result # body result value 
'10213'

wtfrank · 2016-07-27T15:36:20Z

lets get a bit more perverse then...what about this?

{
    "result": { "result" : "10213", status_code : 500},
    "items": [],
    "status_code": { "result" : "10214", status_code: 501},
    "pageCount_str": "1",
    "totalCount": 10213
}

hkraal · 2016-07-27T17:28:07Z

Seems to do what I intended:

self.assertEqual(res.status_code, 200)
self.assertEqual(res.pageCount_str, '1')
self.assertEqual(res.result.pageCount_str, '1')
self.assertEqual(res.totalCount, 10213)
self.assertEqual(res.result.totalCount, 10213)
self.assertEqual(res.items, [])
self.assertEqual(res.result.items, [])
self.assertEqual(res.result.status_code.status_code, 501)
self.assertEqual(res.result.status_code.result, '10214')
self.assertEqual(res.result.result.result, '10213')

Unittest is completed successfully

wtfrank · 2016-07-27T17:31:13Z

haha fair enough so it could work!

hkraal · 2016-07-27T18:03:28Z

Ok... so what would these commits change:

each call foo(), foo().bar() etc will result a dictionary being returned with additional information like the following eve().time() example and will contain the "old" response in .result

>>> eve().time()
{
  'endpoint_version': 'application/vnd.ccp.eve.Time-v1+json',
  'expires_at': 1469641381.3412392,
  'status_code': 200,
  'result': {
    'time': '2016-07-27T17:42:50'
  },
  'expires_in': 10
}

The properties of the call are still accessible via the old way (should we sent out deprecation notices when this get's used?):

>>> eve().time().time
'2016-07-27T17:49:40'

The properties of the call should however be accessed by .result:

>>> eve().time().result.time
'2016-07-27T17:51:50'

In case __call__() returns an href attribute like eve() has marketData and marketData is an property of the response object you will not be able to retrieve the url by calling eve().marketData. This will return the property of the response object instead. This would happen now if we would get an .endpoint_version, .expires_at, .status_code, .result or .expires_in.
Attribute failures haven't changed afaik:

>>> eve().time().localtime
Traceback (most recent call last):
  File "<stdin>", line 1, in <module>
  File "/home/henk/Documents/Projects/pyCrest/src/pycrest/eve.py", line 450, in __getattr__
    return self._dict['result'].__getattr__(item)
  File "/home/henk/Documents/Projects/pyCrest/src/pycrest/eve.py", line 452, in __getattr__
    raise AttributeError(item)
AttributeError: localtime

>>> eve().time().result.localtime
Traceback (most recent call last):
  File "<stdin>", line 1, in <module>
  File "/home/henk/Documents/Projects/pyCrest/src/pycrest/eve.py", line 452, in __getattr__
    raise AttributeError(item)
AttributeError: localtime

You can move deeper into crest (can we get T-shirts with that?) by the response object or .result

>>> root = eve()
>>> root.result.time()
{'endpoint_version': 'application/vnd.ccp.eve.Time-v1+json', 'expires_at': 1469642176.007282, 'status_code': 200, 'result': {'time': '2016-07-27T17:56:00'}, 'expires_in': 10}

>>> root.time()
{'endpoint_version': 'application/vnd.ccp.eve.Time-v1+json', 'expires_at': 1469642187.9248161, 'status_code': 200, 'result': {'time': '2016-07-27T17:56:10'}, 'expires_in': 10}

>>> eve().result.time()
{'endpoint_version': 'application/vnd.ccp.eve.Time-v1+json', 'expires_at': 1469642290.9267902, 'status_code': 200, 'result': {'time': '2016-07-27T17:58:00'}, 'expires_in': 10}

>>> eve().time()
{'endpoint_version': 'application/vnd.ccp.eve.Time-v1+json', 'expires_at': 1469642290.9267902, 'status_code': 200, 'result': {'time': '2016-07-27T17:58:00'}, 'expires_in': 10}

jonobrien · 2016-07-27T19:33:39Z

this might be a good time to pull in versioning (#17) notices since you mentioned deprecation notices.

have you tried pulling market data? If we are saving the entire old response as well, is there going to be a lot of overhead moving around large amounts of data this way, with high item counts?

Or an I just misreading this and we just have to access the response via obj.result now instead of just parsing it directly?

hkraal · 2016-07-27T19:59:52Z

It's shouldn't make a huge difference in performance, there are some extra calls but as these are so amazingly fast I doubt we would ever notice.

Had a chat with @wtfrank on gitter and we ended up not wanting to do this. It's funny to see it works but as he put it:

(08:35:54 PM) wtfrank: I would describe the old behaviour as: "." references the object within the json returned by crest, "()" loads the link specified in an href at the current position of the josn returned by crest
(08:36:34 PM) wtfrank: but in that patch, "." means either a) reference an object within the json returned by crest, or b) reference a magic attribute added in that you have to memorise

I started over and now have the following.

>>> eve = EVE()
>>> eve()
{'userCount_str': '26249', 'marketTypes': {'href': 'https://crest-tq.eveonline.com/market/types/'}, 'constellations': {'href': 'https://crest-tq.eveonline.com/constellations/'}, 'decode': {'href': 'https://crest-tq.eveonline.com/decode/'}, 'virtualGoodStore': {'href': 'https://vgs-tq.eveonline.com/'}, 'itemTypes': {'href': 'https://crest-tq.eveonline.com/inventory/types/'}, 'itemGroups': {'href': 'https://crest-tq.eveonline.com/inventory/groups/'}, 'tournaments': {'href': 'https://crest-tq.eveonline.com/tournaments/'}, 'serverVersion': 'EVE-TRANQUILITY 14.06.1058053.1058052', 'insurancePrices': {'href': 'https://crest-tq.eveonline.com/insuranceprices/'}, 'sovereignty': {'structures': {'href': 'https://crest-tq.eveonline.com/sovereignty/structures/'}, 'campaigns': {'href': 'https://crest-tq.eveonline.com/sovereignty/campaigns/'}}, 'regions': {'href': 'https://crest-tq.eveonline.com/regions/'}, 'userCount': 26249, 'corporations': {'href': 'https://crest-tq.eveonline.com/corporations/'}, 'industry': {'facilities': {'href': 'https://crest-tq.eveonline.com/industry/facilities/'}, 'systems': {'href': 'https://crest-tq.eveonline.com/industry/systems/'}}, 'dogma': {'attributes': {'href': 'https://crest-tq.eveonline.com/dogma/attributes/'}, 'effects': {'href': 'https://crest-tq.eveonline.com/dogma/effects/'}}, 'incursions': {'href': 'https://crest-tq.eveonline.com/incursions/'}, 'systems': {'href': 'https://crest-tq.eveonline.com/solarsystems/'}, 'races': {'href': 'https://crest-tq.eveonline.com/races/'}, 'opportunities': {'tasks': {'href': 'https://crest-tq.eveonline.com/opportunities/tasks/'}, 'groups': {'href': 'https://crest-tq.eveonline.com/opportunities/groups/'}}, 'alliances': {'href': 'https://crest-tq.eveonline.com/alliances/'}, 'authEndpoint': {'href': 'https://login-tq.eveonline.com/oauth/token/'}, 'time': {'href': 'https://crest-tq.eveonline.com/time/'}, 'serverName': 'TRANQUILITY', 'marketGroups': {'href': 'https://crest-tq.eveonline.com/market/groups/'}, 'itemCategories': {'href': 'https://crest-tq.eveonline.com/inventory/categories/'}, 'serviceStatus': 'online', 'bloodlines': {'href': 'https://crest-tq.eveonline.com/bloodlines/'}, 'marketPrices': {'href': 'https://crest-tq.eveonline.com/market/prices/'}, 'wars': {'href': 'https://crest-tq.eveonline.com/wars/'}, 'npcCorporations': {'href': 'https://crest-tq.eveonline.com/corporations/npccorps/'}}
>>> eve()['status_code']
200
>>> eve()['endpoint_version']
application/vnd.ccp.eve.Api-v5+json
>>> eve()['expires_in']
20
>>> eve()['expires_at']
1469648871.1075354

hkraal · 2016-07-27T20:45:32Z

Closing this in favor of #39

Henk Kraal added 2 commits July 26, 2016 16:15

Return a dictionary

14b4325

Comment when we do wierd stuff

237ebe3

hkraal added the enhancement label Jul 26, 2016

hkraal added this to the v0.1.0 milestone Jul 26, 2016

hkraal self-assigned this Jul 26, 2016

Henk Kraal added 2 commits July 26, 2016 16:49

check if "result" exists

267ec5f

include endpoint version

1840fd1

hkraal mentioned this pull request Jul 27, 2016

Cache handles stale values #22

Closed

Add test to demonstrate the lack of namespace issues

10918f6

pervese unittest

756b40c

hkraal mentioned this pull request Jul 27, 2016

Return additional request information - mk2 #39

Closed

hkraal closed this Jul 27, 2016

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

[WIP] Return additional request information #37

[WIP] Return additional request information #37

hkraal commented Jul 26, 2016

jonobrien commented Jul 26, 2016

wtfrank commented Jul 26, 2016 •

edited

Loading

wtfrank commented Jul 26, 2016

hkraal commented Jul 27, 2016

jonobrien commented Jul 27, 2016

hkraal commented Jul 27, 2016

wtfrank commented Jul 27, 2016 •

edited

Loading

jonobrien commented Jul 27, 2016 •

edited

Loading

jonobrien commented Jul 27, 2016

hkraal commented Jul 27, 2016

jonobrien commented Jul 27, 2016

wtfrank commented Jul 27, 2016 •

edited

Loading

hkraal commented Jul 27, 2016

wtfrank commented Jul 27, 2016

wtfrank commented Jul 27, 2016 •

edited

Loading

hkraal commented Jul 27, 2016

wtfrank commented Jul 27, 2016 •

edited

Loading

hkraal commented Jul 27, 2016

wtfrank commented Jul 27, 2016 •

edited

Loading

hkraal commented Jul 27, 2016 •

edited

Loading

wtfrank commented Jul 27, 2016

hkraal commented Jul 27, 2016 •

edited

Loading

jonobrien commented Jul 27, 2016 •

edited

Loading

hkraal commented Jul 27, 2016

hkraal commented Jul 27, 2016

[WIP] Return additional request information #37

[WIP] Return additional request information #37

Conversation

hkraal commented Jul 26, 2016

jonobrien commented Jul 26, 2016

wtfrank commented Jul 26, 2016 • edited Loading

wtfrank commented Jul 26, 2016

hkraal commented Jul 27, 2016

jonobrien commented Jul 27, 2016

hkraal commented Jul 27, 2016

wtfrank commented Jul 27, 2016 • edited Loading

jonobrien commented Jul 27, 2016 • edited Loading

jonobrien commented Jul 27, 2016

hkraal commented Jul 27, 2016

jonobrien commented Jul 27, 2016

wtfrank commented Jul 27, 2016 • edited Loading

hkraal commented Jul 27, 2016

wtfrank commented Jul 27, 2016

wtfrank commented Jul 27, 2016 • edited Loading

hkraal commented Jul 27, 2016

wtfrank commented Jul 27, 2016 • edited Loading

hkraal commented Jul 27, 2016

wtfrank commented Jul 27, 2016 • edited Loading

hkraal commented Jul 27, 2016 • edited Loading

wtfrank commented Jul 27, 2016

hkraal commented Jul 27, 2016 • edited Loading

jonobrien commented Jul 27, 2016 • edited Loading

hkraal commented Jul 27, 2016

hkraal commented Jul 27, 2016

wtfrank commented Jul 26, 2016 •

edited

Loading

wtfrank commented Jul 27, 2016 •

edited

Loading

jonobrien commented Jul 27, 2016 •

edited

Loading

wtfrank commented Jul 27, 2016 •

edited

Loading

wtfrank commented Jul 27, 2016 •

edited

Loading

wtfrank commented Jul 27, 2016 •

edited

Loading

wtfrank commented Jul 27, 2016 •

edited

Loading

hkraal commented Jul 27, 2016 •

edited

Loading

hkraal commented Jul 27, 2016 •

edited

Loading

jonobrien commented Jul 27, 2016 •

edited

Loading