search

lightningdevkit / rust-lightning

A highly modular Bitcoin Lightning library written in Rust. It's rust-lightning, not Rusty's Lightning!
Other
1.15k stars 363 forks source link

Remove usage of blockdata:: from bitcoin paths #3146

Closed tcharding closed 2 months ago

tcharding commented 3 months ago

In rust-bitcoin the blockdata module is a code organisation thing, it should never have been public. One day those guys would like to remove it, so as not to be a PITA for rust-lighting when they do lets remove all usage of blockdata:: now.

Internal change only, no externally visible changes.

tcharding commented 3 months ago

I can't work out what incantation of rustfmt to use to get past CI, can you tell me please. I tried removing --check from ci/rustfmt.sh but got a million unrelated changes?

tcharding commented 3 months ago

As one of the idiots that pushed for rustfmt in rust-bitcoin I'm aware of the irony in my request :)

codecov-commenter commented 3 months ago

Codecov Report

All modified and coverable lines are covered by tests :white_check_mark:

Project coverage is 89.78%. Comparing base (88e1b56) to head (0c19a99).

:exclamation: Your organization needs to install the Codecov GitHub app to enable full functionality.

Additional details and impacted files ```diff @@ Coverage Diff @@ ## main #3146 +/- ## ========================================== - Coverage 89.83% 89.78% -0.05% ========================================== Files 121 121 Lines 98900 98900 Branches 98900 98900 ========================================== - Hits 88847 88802 -45 - Misses 7457 7494 +37 - Partials 2596 2604 +8 ```

:umbrella: View full report in Codecov by Sentry.
:loudspeaker: Have feedback on the report? Share it here.

TheBlueMatt commented 3 months ago

Oh sorry I can't read lol. rustfmt is mad about the ordering of your imports, so you should be able to just reorder imports repeatedly until the CI script passes.

tcharding commented 3 months ago

Sweet, yesterday I thought I'd gone mad but master is broken right now - there are formatting issues on it.

TheBlueMatt commented 3 months ago

I don't believe so? CI on latest git doesn't show any issues on the rustfmt job, the issue here is that removing the blockdata changed the alphabetical order of the imports so rustfmt is mad.

tcharding commented 3 months ago

I'm traveling for the week, I'll come back to this. Thanks

TheBlueMatt commented 2 months ago

Any desire to pick this back up?

tcharding commented 2 months ago

Not if I have to re-order all the import statements by hand, sorry man.

TheBlueMatt commented 2 months ago

You can rustfmt individual files, but alright, up to you 🤷‍♂️

tcharding commented 2 months ago

FWIW I had another go at this and as far as I can tell there are formatting issues on master but reading the workflow I'm confused as to how they got past CI, but you guys have a fair few red PRs right now so I didn't look any further.