Skip to content

Move lightning-invoice deser errors to lib.rs instead of pub use #1409

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

Merged

Conversation

TheBlueMatt
Copy link
Collaborator

Having public types in a private module is somewhat awkward from a
readability standpoint, but, more importantly, the bindings logic
has a relatively rough go of converting them - it doesn't implement
pub use as its "implement this function" logic is all within the
context of a module. We'd need to keep a set of re-exported things
to implement them when parsing modules...or we could just move two
enums from de.rs to lib.rs here, which is substantially less
work.

If we're happy with this I'll open a copy of this against 0.0.106-bindings.

jkczyz
jkczyz previously approved these changes Apr 4, 2022
Copy link
Contributor

@jkczyz jkczyz left a comment

Choose a reason for hiding this comment

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

LGTM

@codecov-commenter
Copy link

codecov-commenter commented Apr 4, 2022

Codecov Report

Merging #1409 (03cafce) into main (0a0f87c) will decrease coverage by 0.01%.
The diff coverage is 11.11%.

@@            Coverage Diff             @@
##             main    #1409      +/-   ##
==========================================
- Coverage   90.83%   90.82%   -0.02%     
==========================================
  Files          73       73              
  Lines       41266    41266              
  Branches    41266    41266              
==========================================
- Hits        37486    37479       -7     
- Misses       3780     3787       +7     
Impacted Files Coverage Δ
lightning-invoice/src/de.rs 82.38% <ø> (+1.11%) ⬆️
lightning-invoice/src/lib.rs 87.39% <11.11%> (-0.85%) ⬇️
lightning/src/ln/functional_tests.rs 97.04% <0.00%> (-0.10%) ⬇️

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update 0a0f87c...03cafce. Read the comment docs.

arik-so
arik-so previously approved these changes Apr 4, 2022
Having public types in a private module is somewhat awkward from a
readability standpoint, but, more importantly, the bindings logic
has a relatively rough go of converting them - it doesn't implement
`pub use` as its "implement this function" logic is all within the
context of a module. We'd need to keep a set of re-exported things
to implement them when parsing modules...or we could just move two
enums from `de.rs` to `lib.rs` here, which is substantially less
work.
@TheBlueMatt TheBlueMatt dismissed stale reviews from arik-so and jkczyz via ce5a9f5 April 4, 2022 13:19
@TheBlueMatt TheBlueMatt force-pushed the 2022-04-bindings-invoice-ders branch from 03cafce to ce5a9f5 Compare April 4, 2022 13:19
@TheBlueMatt
Copy link
Collaborator Author

Oops, sorry, dropped a spurious newline I'd added.

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.

4 participants