Skip to content

[0.0.111-bindings] Add a bindings-only version of Future::register_callback #1736

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

Conversation

TheBlueMatt
Copy link
Collaborator

While we could, in theory, add support to the bindings logic to map
Box<dyn Trait>, there isn't a whole lot of use doing so when its
incredibly trivial to do directly.

This adds a trivial wrapper around Future::register_callback that
is only built in bindings and which is linked in the
register_callback docs for visibility.

This also has a backport of #1734 because why not.

While we could, in theory, add support to the bindings logic to map
`Box<dyn Trait>`, there isn't a whole lot of use doing so when its
incredibly trivial to do directly.

This adds a trivial wrapper around `Future::register_callback` that
is only built in bindings and which is linked in the
`register_callback` docs for visibility.
as the first is quite a mouthful, and the second isn't materially
less specific.
@TheBlueMatt
Copy link
Collaborator Author

Oops, right, this should go upstream first. Please bikeshed the function name at #1737 instead and we should land that first.

@codecov-commenter
Copy link

codecov-commenter commented Sep 23, 2022

Codecov Report

Base: 90.75% // Head: 90.78% // Increases project coverage by +0.03% 🎉

Coverage data is based on head (e498e22) compared to base (b4a40f6).
Patch has no changes to coverable lines.

Additional details and impacted files
@@                 Coverage Diff                  @@
##           0.0.111-bindings    #1736      +/-   ##
====================================================
+ Coverage             90.75%   90.78%   +0.03%     
====================================================
  Files                    86       86              
  Lines                 46530    46530              
  Branches              46530    46530              
====================================================
+ Hits                  42227    42242      +15     
+ Misses                 4303     4288      -15     
Impacted Files Coverage Δ
lightning/src/routing/scoring.rs 96.14% <ø> (ø)
lightning/src/util/wakers.rs 86.70% <ø> (ø)
lightning/src/ln/payment_tests.rs 98.82% <0.00%> (+0.13%) ⬆️
lightning/src/ln/functional_tests.rs 96.98% <0.00%> (+0.19%) ⬆️
lightning-net-tokio/src/lib.rs 77.03% <0.00%> (+0.30%) ⬆️

Help us with your feedback. Take ten seconds to tell us how you rate us. Have a feature suggestion? Share it here.

☔ View full report at Codecov.
📢 Do you have feedback about the report comment? Let us know in this issue.

@dunxen
Copy link
Contributor

dunxen commented Sep 24, 2022

Is the rename from #1734, not a breaking change? Or are we okay with that?

@TheBlueMatt
Copy link
Collaborator Author

Yea, it's not breaking, but we also backported the adoption of the struct itself, and it seems more annoyance than it's worth to ship a struct and then rename it in the next release.

@TheBlueMatt TheBlueMatt merged commit 5b5908a into lightningdevkit:0.0.111-bindings Sep 25, 2022
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants