Qualify BOLT 11/12 invoice and related types #2416
Conversation
|
Probably should rename |
Codecov ReportPatch coverage:
❗ Your organization is not using the GitHub App Integration. As a result you may experience degraded service beginning May 15th. Please install the Github App Integration for your organization. Read more. Additional details and impacted files@@ Coverage Diff @@
## main #2416 +/- ##
==========================================
+ Coverage 90.23% 90.24% +0.01%
==========================================
Files 106 106
Lines 55213 55442 +229
Branches 55213 55442 +229
==========================================
+ Hits 49819 50035 +216
- Misses 5394 5407 +13
☔ View full report in Codecov by Sentry. |
|
Lol, cargo check hit an ICE in CI. I kicked it lets see if retry fixes it. |
|
I'd really love to do this for 0.0.116, even though its quite last-minute - it reduces the burden on the bindings and lets us expose the BOLT12 stuff as-is, which would be very nice. |
|
Grr, yea, its reliable, and I can repro it but not with latest rustc. Can you just swap the rustc used for the |
| @@ -17,27 +17,27 @@ use crate::util::string::UntrustedString; | |||
|
|
|||
| use crate::prelude::*; | |||
|
|
|||
| /// An error in response to an [`InvoiceRequest`] or an [`Invoice`]. | |||
| /// An error in response to an [`InvoiceRequest`] or an [`Bolt12Invoice`]. | |||
There was a problem hiding this comment.
What if we just renamed Bolt11Invoices and left Bolt12Invoice as Invoice? That way we don't have ambiguity with InvoiceRequest and makes it clearer what's being deprecated
There was a problem hiding this comment.
The type is pretty ambiguous if we just do one, if we're worried about the mismatch I'd vote we rename all of the types, but I don't feel that strongly.
There was a problem hiding this comment.
Would prefer to just qualify both Invoice types. InvoiceRequest is sorta borderline for me, but I'm leaning to not renaming it. There's no name clash there and it's a higher-level type rather than being an auxiliary type for Invoice.
| @@ -18,11 +18,11 @@ | |||
| //! invoices and functions to create, encode and decode these. If you just want to use the standard | |||
| //! en-/decoding functionality this should get you started: | |||
| //! | |||
| //! * For parsing use `str::parse::<Invoice>(&self)` (see [`Invoice::from_str`]) | |||
| //! * For parsing use `str::parse::<Bolt11Invoice>(&self)` (see [`Bolt11Invoice::from_str`]) | |||
| //! * For constructing invoices use the [`InvoiceBuilder`] | |||
There was a problem hiding this comment.
There's still two InvoiceBuilder structs, though I guess those aren't exposed in bindings anyway?
There's also:
InvoiceSignatureInvoiceDescription
not sure if those should be disambiguated
There was a problem hiding this comment.
There's still two
InvoiceBuilderstructs, though I guess those aren't exposed in bindings anyway?
Yeah, as noted in the PR description, those should probably be renamed but currently aren't exposed so I opted not to.
There's also:
InvoiceSignatureInvoiceDescription
not sure if those should be disambiguated
This is borderline. On one hand, they are part of a type that is already qualified so it seems redundant. On the other hand, they are public types. I'm somewhat indifferent here, so I can change those if you prefer.
There was a problem hiding this comment.
I thought bindings users lose module prefixes so if that's the case I'd be in favor of renaming. Nbd though.
There was a problem hiding this comment.
Are you referring to InvoiceBuilder? I was agreeing with that but just saying not now since they aren't currently exported in bindings. We can do it now if you prefer, though.
There was a problem hiding this comment.
Oh sorry, no I meant the description and signature.
There was a problem hiding this comment.
Gotcha. Renamed now.
Otherwise, the compiler will panic when using 1.57 for upcoming commits.
To avoid a naming conflict in bindings with BOLT 11 invoices, qualify the BOLT 12 invoice type.
A previous commit qualified the BOLT 12 invoice type, so any related types should be similarly qualified, if public.
To avoid a naming conflict in bindings with BOLT 11 parse error, qualify the BOLT 12 parse error type.
To avoid a naming conflict in bindings with BOLT 11 semantic error, qualify the BOLT 12 semantic error type.
A previous commit qualified the BOLT 12 invoice type. Qualify the BOLT 11 invoice type for consistency.
A previous commit qualified the BOLT 11 invoice type, so any related types should be similarly qualified, if public.
A previous commit qualified the BOLT 12 parse error type. Qualify the BOLT 11 parse error type for consistency.
A previous commit qualified the BOLT 12 semantic error type. Qualify the BOLT 11 semantic error type for consistency.
jkczyz
force-pushed
the
2023-07-invoice-rename
branch
from
1deaf26 to
57d9f22
Compare
Went ahead and renamed |
A previous commit qualified the BOLT 11 invoice type, so any related types should be similarly qualified, if public.
jkczyz
force-pushed
the
2023-07-invoice-rename
branch
from
57d9f22 to
8c4a85b
Compare
A previous commit qualified the BOLT 11 invoice type, so any related types should be similarly qualified, if public.
A previous commit qualified the BOLT 11 invoice type, so any related types should be similarly qualified, if public.
Qualifies BOLT 11/12 types such that there are no conflicts in bindings. This includes public types like
Invoiceand similar types (e.g.,RawInvoice,SignedRawInvoice, andUnsignedInvoice) and error types (ParseErrorandSemanticError). However, it does not include types likeInvoiceRequest,InvoiceDescription,InvoiceSignature, etc. nor types likeParseOrSemanticError, though there may be an argument for renaming some of these types, too.Private types were not renamed.
InvoiceBuildertypes have not been renamed yet since they are not exported for bindings. Though they probably should be for consistency with theInvoicerename. Open to doing that in this PR.