Skip to content
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.

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

Enhances how the SDK handles API error messages #338

Open
wants to merge 13 commits into
base: master
Choose a base branch
from

Conversation

alessiocastrica
Copy link
Contributor

No description provided.

@alessiocastrica alessiocastrica linked an issue Sep 20, 2023 that may be closed by this pull request
2 tasks
@alessiocastrica alessiocastrica changed the title Error messages Enhances how the SDK handles API error messages Sep 20, 2023
@alessiocastrica alessiocastrica marked this pull request as ready for review September 21, 2023 17:55
@alessiocastrica alessiocastrica force-pushed the error-messages branch 2 times, most recently from 68527b6 to 032a78b Compare October 6, 2023 09:51
@alessiocastrica alessiocastrica requested review from Neal and removed request for haxdds October 6, 2023 12:13
Comment on lines 527 to 529
assert (
"id" in last_result
), "AccountActivity didn't contain an `id` field to use for paginating results"
Copy link
Contributor

Choose a reason for hiding this comment

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

assert is ignored if optimization is requested. Therefore, if we need to handle an error, we should not use assert as a error handling method.

ref. https://docs.python.org/3.12/reference/simple_stmts.html#the-assert-statement

@@ -1,4 +1,4 @@
from pydantic import BaseModel
from pydantic import BaseModel, ConfigDict
Copy link
Contributor

Choose a reason for hiding this comment

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

ConfigDict could be removed?

@@ -525,7 +525,7 @@ def _get_account_activities_iterator(
last_result = result[-1]

if "id" not in last_result:
raise APIError(
raise AssertionError(
Copy link
Contributor

Choose a reason for hiding this comment

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

built-in AssertionError is defined as Raised when an assert statement fails..
Could be something different exception?

ref. https://docs.python.org/3/library/exceptions.html#AssertionError

Copy link
Contributor Author

@alessiocastrica alessiocastrica Oct 10, 2023

Choose a reason for hiding this comment

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

Maybe we can raise ValueError or AttributeError? Which one it's better in your opinion? I'd go with the attribute error based on the docs

# we got here either by error or someone has mis-configured us, so we didn't even try
raise Exception("Somehow we never made a request for download!")
# we got here either by error or someone has mis-configured us, so we didn't even try
assert isinstance(
Copy link
Contributor

Choose a reason for hiding this comment

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

this assert also can be replaceable?

Copy link
Contributor Author

@alessiocastrica alessiocastrica Oct 10, 2023

Choose a reason for hiding this comment

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

I'd raise a TypeError here, sounds good?

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.

[Bug]: Error messages not propagated
2 participants