Skip to content

Conversation

@TheBlueMatt
Copy link
Collaborator

I wrote this when debugging a user's scorer and figured it may be
useful upstream.

rloomba
rloomba previously approved these changes Apr 28, 2022
@codecov-commenter
Copy link

codecov-commenter commented Apr 28, 2022

Codecov Report

Merging #1460 (ea442e1) into main (62edee5) will increase coverage by 1.14%.
The diff coverage is n/a.

@@            Coverage Diff             @@
##             main    #1460      +/-   ##
==========================================
+ Coverage   90.88%   92.03%   +1.14%     
==========================================
  Files          75       75              
  Lines       41517    46637    +5120     
  Branches    41517    46637    +5120     
==========================================
+ Hits        37734    42921    +5187     
+ Misses       3783     3716      -67     
Impacted Files Coverage Δ
lightning/src/routing/scoring.rs 94.36% <ø> (ø)
lightning/src/util/config.rs 45.05% <0.00%> (-0.78%) ⬇️
lightning/src/ln/features.rs 99.59% <0.00%> (+0.15%) ⬆️
lightning/src/ln/functional_test_utils.rs 95.96% <0.00%> (+0.40%) ⬆️
lightning/src/ln/functional_tests.rs 98.52% <0.00%> (+1.37%) ⬆️
lightning/src/ln/channel.rs 92.08% <0.00%> (+3.65%) ⬆️
lightning/src/ln/channelmanager.rs 88.66% <0.00%> (+3.91%) ⬆️

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 62edee5...ea442e1. Read the comment docs.

@jkczyz jkczyz self-requested a review April 29, 2022 05:06
Copy link
Contributor

@vincenzopalazzo vincenzopalazzo left a comment

Choose a reason for hiding this comment

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

LGTM.

Question: There is any way from the application layer to disable the log? dummy question,, sorry

dunxen
dunxen previously approved these changes Apr 29, 2022
@TheBlueMatt TheBlueMatt dismissed stale reviews from dunxen and vincenzopalazzo via d254510 April 29, 2022 20:07
dunxen
dunxen previously approved these changes Apr 29, 2022
Copy link
Contributor

@dunxen dunxen left a comment

Choose a reason for hiding this comment

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

ACK d254510

jkczyz
jkczyz previously approved these changes May 2, 2022
@jkczyz
Copy link
Contributor

jkczyz commented May 2, 2022

LGTM.

Question: There is any way from the application layer to disable the log? dummy question,, sorry

The function isn't called anywhere, so an application would have to explicitly call it. In general, logging is configured using rust features:

# Unlog messages superior at targeted level.

@TheBlueMatt The comment there is a little confusing. Perhaps it could be reworded?

@TheBlueMatt
Copy link
Collaborator Author

@TheBlueMatt The comment there is a little confusing. Perhaps it could be reworded?

Agreed, those should be reworded. Really they need to be copied into the library-level lib.rs docs so that we have a user-visible explanation of the features we have, not just comments in the Cargo.toml. I'll open an issue.

I wrote this when debugging a user's scorer and figured it may be
useful upstream.
@TheBlueMatt
Copy link
Collaborator Author

Squashed fixup commits.

@arik-so
Copy link
Contributor

arik-so commented May 3, 2022

Given this function isn't called or tested anywhere, are we running the risk of deleting it later on because it looks like dead code? Should it perhaps have at least one call site somewhere?

let graph = self.network_graph.read_only();
for (scid, liq) in self.channel_liquidities.iter() {
if let Some(chan_debug) = graph.channels().get(scid) {
let log_direction = |source, target| {
Copy link
Contributor

Choose a reason for hiding this comment

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

thanks for not making this a macro :)

Copy link
Contributor

Choose a reason for hiding this comment

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

I'm often in macro debugging hell and I hate it :(

@TheBlueMatt
Copy link
Collaborator Author

I don't think we'd just delete something that's a pub fn? The same could be said for most pub fns, no? You could argue about testing, but its just a test util and it should fail to compile in some way if we change the API.

@TheBlueMatt TheBlueMatt merged commit 5feef25 into lightningdevkit:main May 3, 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.

7 participants