-
Notifications
You must be signed in to change notification settings - Fork 2.2k
Update wtclient.go #9588
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
Update wtclient.go #9588
Conversation
Important Review skippedAuto reviews are limited to specific labels. 🏷️ Labels to auto review (1)
Please check the settings in the CodeRabbit UI or the You can disable this status message by setting the Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. 🪧 TipsChatThere are 3 ways to chat with CodeRabbit:
Note: Be mindful of the bot's finite context window. It's strongly recommended to break down tasks such as reading entire modules into smaller chunks. For a focused discussion, use review comments to chat about specific files and their changes, instead of using the PR comments. CodeRabbit Commands (Invoked using PR comments)
Other keywords and placeholders
CodeRabbit Configuration File (
|
thanks for the contribution @MKVEERENDRA , Please read through our contribution guidelines: Contribution Guidelines where you will find info on: how to structure commits & how to name commits 🙏 |
utACK |
@ellemouton ok I will do that to further PR. |
@dennisreimann can you pls review my PR. |
what's the difference between this and #9584? |
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.
Ok
As mentioned above and in this comment, PRs must follow the contribution guidelines. |
Will anyone on the team continue to follow up on this PR? Issue related #9583 |
Haven't seen any update after the last review comment. Ball's in @MKVEERENDRA 's court |
If this guy never responds again, will no one from the Lightning Labs team take care of it? |
@twofaktor - it's not a high prio issue & also is a really nice first-contribution project, so ideally would like to leave it for someone looking to make their first contribution to lnd. |
Hey @ellemouton @twofaktor 👋 Apologies for the delay. I'm ready to wrap this up and would love to get this PR merged. Let me know if there's anything else you'd like me to update or improve. Happy to make any recommended changes. Thanks! |
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.
utACK
@ellemouton could you review this PR? |
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.
ACK with a small suggestion:
The changes improve the CLI help text by making descriptions clearer and more informative. The formatting is correct and the changes are focused.
One small suggestion: In the terminateSessionCommand
, changing ArgsUsage: "id"
to ArgsUsage: "session_id"
would make it even clearer what kind of ID is expected, consistent with the improved clarity in the rest of the changes.
Overall, good improvements to the CLI documentation!
Thanks! @AbhinavAnand241201 Appreciate the ACK and the helpful suggestion. You're absolutely right — changing ArgsUsage: "id" to ArgsUsage: "session_id" makes the help text more explicit and aligned with the rest of the improvements. Just pushed that update 🙌 Let me know if you spot anything else! |
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.
i noticed u worked on the optional improvement,... i suggested upon .. ....
everything else looks fine ..
👍🏻
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.
@MKVEERENDRA
As requested a couple times before, please read our contribution guidelines regarding how to go about structuring commits and titling commits. Also, I think previously I made this comment on another PR but then you closed that in order to open this one. Rather just push changes to this PR instead of opening a new on. The changes in this PR can really all just be in a single commit.
… fixes. - Added detailed descriptions for the session and terminate commands in wtclient to improve user guidance and clarity. - Changed the ArgsUsage for the session command from 'id' to 'session_id'. - Refined descriptions to explain the purpose of each command. - Fixed long description lines to adhere to the 80-character line limit. - Updated the release notes for version 0.19.0. Fixes: lightningnetwork#9583
@yyforyongyu Why was the PR closed without merging? There were unmerged commits — should we reopen it or create a new one? |
@saubyk can you reopen it or please clarify that like why it was closed. |
@MKVEERENDRA - i've reopened it |
@MKVEERENDRA - note that commit message structure has still not been followed here. please read our guidelines carefully |
@ellemouton "note that commit message structure has still not been followed here. please read our guidelines carefully" -- Can you pls elaborate that what I not been followed, I am bit confused like I read the guidelines carefully and did many changes like detail commit messages and strink commit ..etc, but I did not find any difference like what is wrong. |
@MKVEERENDRA - see here |
@ellemouton: review reminder |
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.
please re-request once updated
It is being worked on in parallel PR #9765 which seems more appropriate |
In that case, can we close this pr now? |
Alright, if PR #9765 is more appropriate, let's go ahead and close this one. The reason I initially opened this PR was to contribute the changes quickly, but since it's now being handled better in parallel, I'm happy to close it here. Also, @ellemouton — you mentioned "commit message structure has still not been followed." I did read the contribution guidelines carefully and tried to apply them (added detailed messages, squashed commits, etc.), so I’m not exactly sure what I missed. Could you please help clarify what specifically was incorrect? That would help me a lot to improve in the future. Thanks! |
@MKVEERENDRA - did you see the link I posted above? this one? basically you need to have a short (max 50 char title) that starts with the package being touched in that commit and then you can follow it with a description body that wraps at 80. So a title for this commit could have been: |
@ellemouton Ohh now I get it, Tq for the information. |
wtclient: Add descriptions for session and terminate commands
Improved user guidance by adding detailed CLI help text for the session and terminate subcommands in the watchtower client (wtclient).
Fixed formatting issues flagged by the linter.
Fixes: #9583
Change Description
Adds helpful descriptions for lncli wtclient session and lncli wtclient terminate to make the CLI more user-friendly. No logic changes.
Steps to Test
lncli wtclient
Check that the session and terminate commands now have clear descriptions.
make lint
PR Checklist
Passes all CI checks
Follows code and doc style (80 char lines, proper formatting)
Commits follow ideal structure
Not a typo-only change
No tests required (CLI help text only)
No release notes change needed