Skip to content

Improve ApiError message formatting; add response field to ApiError and UnexpectedResponseError #467

New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Jump to bottom
Merged
lgarber-akamai merged 5 commits into from
Oct 28, 2024
Merged

Improve ApiError message formatting; add response field to ApiError and UnexpectedResponseError #467

lgarber-akamai merged 5 commits into from
Oct 28, 2024

Conversation

@lgarber-akamai
Copy link
Contributor

@lgarber-akamai lgarber-akamai commented Oct 14, 2024 •
edited
Loading

📝 Description

This pull request improves the ApiError message formatting logic to include the method and path of the failed request. This is done through a new ApiError.from_request(...) class method, which as a bonus simplifies and unifies API error construction across the project.

✔️ How to Test

The following test steps assume you have pulled down this PR locally and run make install.

Unit Testing

make testunit

Integration Testing

make testint

Manual Testing

  1. In a linode_api4-python sandbox environment (e.g. dx-devenv), run the following:
import os

from linode_api4 import LinodeClient, ApiError

# Ensure non-API errors are handled gracefully
try:
    LinodeClient(token="foobar", base_url="https://example.com").get("/fake/endpoint")
except ApiError as exc:
    print(exc)

# Ensure bad token errors are handled gracefully
try:
    LinodeClient(token="foobar").linode.instances()
except ApiError as exc:
    print(exc)

client = LinodeClient(token=os.getenv("LINODE_TOKEN"))

# Ensure invalid endpoint errors are handled gracefully
try:
    client.get("/fake/endpoint")
except ApiError as exc:
    print(exc)

# Ensure invalid request errors are handled gracefully
try:
    client.linode.instance_create("fake-type", "fake-region")
except ApiError as exc:
    print(exc)
  1. Ensure the output matches the following:
GET /fake/endpoint: [404] <!doctype html>
<html>
...
</html>

GET /v4/linode/instances: [401] Invalid Token
GET /v4/fake/endpoint: [404] Not found
POST /v4/linode/instances: [400] type: A valid plan type by that ID was not found; region: region is not valid

@lgarber-akamai lgarber-akamai added the improvement for improvements in existing functionality in the changelog. label Oct 14, 2024
lgarber-akamai
lgarber-akamai commented Oct 14, 2024
View reviewed changes
linode_api4/errors.py
self.errors = [e["reason"] for e in json["errors"]]

@classmethod
def from_response(
Copy link
Contributor Author

@lgarber-akamai lgarber-akamai Oct 14, 2024

Choose a reason for hiding this comment

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

I originally wanted to implement the formatting logic using a __str__ override but instead opted for this implementation to reduce the chances of a breaking change.

@lgarber-akamai lgarber-akamai marked this pull request as ready for review October 14, 2024 18:53
@lgarber-akamai lgarber-akamai requested a review from a team as a code owner October 14, 2024 18:53
@lgarber-akamai lgarber-akamai requested review from ezilber-akamai and ykim-akamai and removed request for a team October 14, 2024 18:53
ezilber-akamai
ezilber-akamai reviewed Oct 15, 2024
View reviewed changes
ezilber-akamai
ezilber-akamai approved these changes Oct 15, 2024
View reviewed changes
Copy link
Contributor

@ezilber-akamai ezilber-akamai left a comment

Choose a reason for hiding this comment

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

Everything is passing locally!

ykim-akamai
ykim-akamai approved these changes Oct 25, 2024
View reviewed changes
Copy link
Contributor

@ykim-akamai ykim-akamai left a comment

Choose a reason for hiding this comment

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

LGTM, tests passed locally

@lgarber-akamai lgarber-akamai merged commit 60622fc into linode:dev Oct 28, 2024
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Reviewers

@ykim-akamai ykim-akamai ykim-akamai approved these changes

@ezilber-akamai ezilber-akamai ezilber-akamai approved these changes

Assignees

No one assigned

Labels

improvement for improvements in existing functionality in the changelog.

Projects

None yet

Milestone

No milestone

Development

Successfully merging this pull request may close these issues.

3 participants

@lgarber-akamai @ykim-akamai @ezilber-akamai