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

Use @property for Response.is_successful (or just make it an attribute) #192

Closed
mDuo13 opened this issue Mar 24, 2021 · 1 comment
Closed
Labels
breaking Changes that would break existing users enhancement New feature or request

Comments

@mDuo13
Copy link
Collaborator

mDuo13 commented Mar 24, 2021

Can you spot what's wrong with the following code?

prelim_result = xrpl.transaction.submit_transaction_blob(blob, client)
print("Preliminary transaction result:", prelim_result)
if not prelim_result.is_successful:
    exit("Submit failed.")

It's not obvious, but is_successful is a method, so it'll always evaluate as truthy; you'll never run the exit() call here. The correct syntax would've been if not prelim_result.is_successful().

We could make is_successful a property using the @property decorator, or maybe just evaluate it at the time the model is instantiated. With that change, result.is_successful would return the correct boolean value. If someone got it wrong and tried result.is_successful() that would raise an error (TypeError: 'bool' object is not callable), which is much better than the sneaky "always true" bug from the above example.

@mDuo13 mDuo13 added breaking Changes that would break existing users enhancement New feature or request labels Mar 24, 2021
@mDuo13 mDuo13 changed the title [FEATURE] [BREAKING] Use @property for Response.is_successful (or just make it an attribute) Use @property for Response.is_successful (or just make it an attribute) Mar 24, 2021
@ledhed2222
Copy link
Collaborator

Closing since we will do the solution described in 202. It doesn't make sense to me that this boolean method should be exposed as a property while many others in our lib would not.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
breaking Changes that would break existing users enhancement New feature or request
Projects
None yet
Development

No branches or pull requests

2 participants