Fix: Error Calling DNS#QOUTAS Issue: #1650

[ojengwa]

I documented the error in the issue.

[dhermes]

Thanks a lot @ojengwa! This LGTM but I'll let @tseaver weigh in.

[tseaver]

The point of Client.quotas is to return actual quota information, which is why the values are converted to ints. The kind key in the server response is irrelevant to the caller: we should just exclude it.

[ojengwa]

That makes sense, of course. However, I have two concerns:

1. Filtering out certain fields presupposes that the API definition might not change (at least anytime soon). Any change that introduces fields that could not be casted as integers would like that the method's internals would need to be updated to reflect such changes.
2. This could potentialy break projects that might depend on the excluded keys (can't see why, but someone might be using the kind field as a filter/check for example).

[dhermes]

@ojengwa Since the API is versioned, the response shouldn't change over time. Also the kind is just a marker for the schema, so it will never change.

[tseaver]

@ojengwa Thanks for the updates!

• Mutating the quotas dict via unconditional pop('kind') fails if the kind key is missing (as in the current breaking test).
• Seems like it would be simpler to change the list comprehension to skip the kind key, e,g.:

returndict([(key, int(value))
forkey, valueinresp['quota'].items()
ifkey!='kind'])

• We probably need to test both cases (with and without kind in the quotas mapping).

[ojengwa]

I guess that works. Have being scratching my head tryna figure out why the kind field shows up in my local tests but not when the test is run on the CI server.

[tseaver]

Uncommitted local changes? The files changed page shows only the one file.

[ojengwa]

Nope. There's nothing to change again in the test case.

[tseaver]

@ojengwaf690398 had a change which injected kind into the quotas dict constructed by the testcase. I was speculating that you still had that change locally, which would explain why the pop didn't raise a KeyError.

[ojengwa]

The KeyError only occurred on Travis. Actually, it was strange. When I examine the return value from Client#quotas locally, I get the kind as a key of the response object as well as a subfield of response['quota'] and the tests ran. But on Travis, it raises a KeyError because it can't find the 'kind' key as a subfield of response['quota'] when trying to call resp['quota'].pop('kind').

[dhermes]

@tseaver Are we all good here?

@ojengwa Do you mind squashing your 10 commits into a single commit? (I'd be happy to lend a hand if needed.)

[tseaver]

Still unaddressed AFAICT:

We probably need to test both cases (with and without kind in the quotas mapping).

[ojengwa]

I have squashed the commits.

[ojengwa]

I don't see how that could work since we are already filtering out kind from the Client#quotas output.

We probably need to test both cases (with and without kind in the quotas mapping).

[tseaver]

@ojengwa I added the test I was asking for in 247e293.