Skip to content

Conversation

@jrfernandes
Copy link

Apparently, result is considered optional in some endpoint responses, such as getting program details:

ResponseOfProgramResponse {
    errors (Array[Error], optional),
    requestId (string, optional),
    result (Array[ProgramResponse], optional),
    success (boolean, optional),
    warnings (Array[string], optional)
}

http://developers.marketo.com/rest-api/endpoint-reference/asset-endpoint-reference/#!/Programs/getProgramByNameUsingGET

I did check a couple of endpoints in the documentation and this is not true for all of them, but, to be safe, this PR handles possible KeyError exceptions and returns an empty list as default.

@jrfernandes jrfernandes self-assigned this Jan 23, 2018
@jrfernandes jrfernandes requested a review from abendig January 23, 2018 18:41
@abendig
Copy link

abendig commented Jan 23, 2018

Can you please add test coverage?

@jrfernandes
Copy link
Author

Done.

@abendig
Copy link

abendig commented Jan 23, 2018

Man, the test coverage in that package is at least as bad as the functionality provided ...

Copy link

@abendig abendig left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Changes look safe and good.

@jrfernandes
Copy link
Author

Ahaha. True that. Might be useful, one day, to start our own cleaned library. Anyway, there's one last commit here to bump the package version in order for pip to install it in case the old version is already installed.

I'll probably open a PR for the original repo, eventually.

@jrfernandes jrfernandes merged commit d76fee9 into master Jan 23, 2018
@jrfernandes jrfernandes deleted the fix_keyerror_exception branch January 23, 2018 23:40
@abendig
Copy link

abendig commented Jan 23, 2018

Would you mind adding this project to Slack (and ideally CI, though I don't know if that's easily possible)?

@jrfernandes
Copy link
Author

I added this repo to our Slack integration. Regarding CI, I'll see about adding a free version of CircleCI here.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

3 participants