-
-
Notifications
You must be signed in to change notification settings - Fork 326
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
refactor: dev-friendly payment status name #2322
Conversation
Codecov ReportAttention: Patch coverage is
Additional details and impacted files@@ Coverage Diff @@
## dev #2322 +/- ##
=======================================
Coverage 58.55% 58.55%
=======================================
Files 61 61
Lines 9204 9210 +6
=======================================
+ Hits 5389 5393 +4
- Misses 3815 3817 +2 ☔ View full report in Codecov by Sentry. |
There are still lots of places like these used in almost all wallet implementations. Examples: (cliche.py) statuses = {"pending": None, "complete": True, "failed": False}
return PaymentStatus(statuses[data["result"]["status"]]) (alby.py) statuses = {
"CREATED": None,
"SETTLED": True,
}
return PaymentStatus(statuses[data.get("state")], fee_msat=None, preimage=None) These deserve to be refactored too because readability of such constructs is horrible. |
575bc51
to
c902073
Compare
This is a good idea, but it is a bit more work. I want to keep this PR easy to review and merge. |
Fair enough 👍 One nitpick though: I find PaymentStatusPending, PaymentStatusFailed and PaymentStatusSuccess better than PaymentPendingStatus, PaymentFailedStatus and PaymentSuccessStatus. Any thoughts? |
I agree, will be added to the #2323 together with |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
utACK
The first parameter of
PaymentStatus
indicates the actual state (success=True
,failed=False
,pending=None
).This is hard to read and remember.
This PR uses 3 new explicit subclasses: