Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
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
[P2P] [Tooling] Peer discovery
peer list
subcommand #892base: main
Are you sure you want to change the base?
[P2P] [Tooling] Peer discovery
peer list
subcommand #892Changes from 1 commit
4badf3a
2b83d32
eac7695
bca000b
5e963be
a883f08
83c3604
6ecca53
d570b35
e952365
e80843c
8f90e22
6e691cd
430db08
440b59a
fcfa837
04dc0aa
3925c71
1bbad38
64abbc0
d8b6296
9ecc9e5
1cbc249
ffbc539
0cff1d2
39af37c
c22011c
1fc2bb4
764e171
9eb5a7e
7380260
c03aa27
da62de1
12e22e1
f8f5da5
66a1347
870805f
64ec990
ccec195
90385f0
6d0d300
c6488a5
2317d42
File filter
Filter by extension
Conversations
Jump to
peer list
subcommandThere are no files selected for viewing
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
See my comments in #801. They're similar to this as well
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@Olshansk can you clarify which comment you are referring to? And what you think should change here?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
E.g. #801 (comment)
@h5law Can you take a stab at #801 first? I (accidently) reviewed it before this one so I avoid repeating some of the stylistic recommendations here.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@Olshansk the reason I am tackling this first is that this is the base of #801 as such I feel it would be difficult to address #801 when the changes may need to be downwards propagated. Will go through #801 alongside from now on
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Thank you. This one is on me
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Have gone through #801 and I think I have already addressed most of the relavent comments in this PR as is
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
debug tag?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
adding
//go:build debug
causes the CLI to break, my guess is its not build with the debug tag? Will look into this furtherThere was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Doing a
ctrl f
forgo build
is a good starting point here.There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
So the reason these cannot be put under debug is as follows. The cli calls the
PrintPeerList
function. So thepeer
subdir in the cli would also need to be with the debug tag. This calls the helper functions so that would need to be in a debug tag and thus all of the CLI would also need it.I think it better to change the
debug
dir to something likeprinting
orintrospect
perhaps.WDYT @Olshansk