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

Rename SpendableOutputDescriptor::outpoint() #3634

Merged
merged 1 commit into from
Mar 3, 2025

Conversation

TheBlueMatt
Copy link
Collaborator

We support some languages (okay, just JavaScript) where functions and fields exist in the same namespace. Sadly, because we map enums as base classes with child classes that add additional fields, this requires that fields in enum variants do not have the same name as functions implemented on that enum.

We violated this in 0.1.1 with
SpendableOutputDescriptor::outpoint which aliases the outpoint fields on two SpendableOutputDescriptor variants.

Here we rename the new outpoint method, which we'll have to carry on the 0.1-bindings branch in addition to going in 0.2.

We support some languages (okay, just JavaScript) where functions
and fields exist in the same namespace. Sadly, because we map
enums as base classes with child classes that add additional
fields, this requires that fields in enum variants do not have the
same name as functions implemented on that enum.

We violated this in 0.1.1 with
`SpendableOutputDescriptor::outpoint` which aliases the `outpoint`
fields on two `SpendableOutputDescriptor` variants.

Here we rename the new `outpoint` method, which we'll have to carry
on the 0.1-bindings branch in addition to going in 0.2.
Copy link

codecov bot commented Mar 2, 2025

Codecov Report

All modified and coverable lines are covered by tests ✅

Project coverage is 89.19%. Comparing base (36ba27a) to head (74018b0).
Report is 2 commits behind head on main.

Additional details and impacted files
@@            Coverage Diff             @@
##             main    #3634      +/-   ##
==========================================
- Coverage   89.20%   89.19%   -0.01%     
==========================================
  Files         152      152              
  Lines      118681   118681              
  Branches   118681   118681              
==========================================
- Hits       105871   105860      -11     
- Misses      10225    10239      +14     
+ Partials     2585     2582       -3     

☔ View full report in Codecov by Sentry.
📢 Have feedback on the report? Share it here.

@ldk-reviews-bot
Copy link

ldk-reviews-bot commented Mar 2, 2025

I've assigned @arik-so as a reviewer!
I'll wait for their review and will help manage the review process.
Once they submit their review, I'll check if a second reviewer would be helpful.

@ldk-reviews-bot ldk-reviews-bot requested a review from arik-so March 2, 2025 20:10
@ldk-reviews-bot
Copy link

👋 The first review has been submitted!

Do you think this PR is ready for a second reviewer? If so, click here to assign a second reviewer.

@ldk-reviews-bot
Copy link

✅ Added second reviewer: @valentinewallace

Copy link
Contributor

@tnull tnull left a comment

Choose a reason for hiding this comment

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

Pity, as it conflicts with Rust's API guidelines.

LGTM though.

@tnull tnull merged commit 47cb737 into lightningdevkit:main Mar 3, 2025
23 of 26 checks passed
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.

5 participants