Skip to content
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

docs/comprehensive lncli #8181

Merged

Conversation

ProofOfKeags
Copy link
Collaborator

@ProofOfKeags ProofOfKeags commented Nov 15, 2023

Change Description

Adds necessary tags to make all currently available lncli commands known to the API documentation tool. Fixes #8102

NOTE: this does not add any sort of checker to guarantee that these tags are always added. Such a facility would likely require a golang AST library and I'm not sure it's worth the work.

Steps to Test

Build the API documentation against this branch/commit and see if all available commands appear in the docs. I did this by hand. Doing this automatically would also require an AST traversing library.

Pull Request Checklist

Testing

  • Your PR passes all CI checks.
  • Tests covering the positive and negative (error paths) are included.
  • Bug fixes contain tests triggering the bug to prevent regressions.

Code Style and Documentation

📝 Please see our Contribution Guidelines for further guidance.

@ProofOfKeags ProofOfKeags requested review from ziggie1984, guggero and saubyk and removed request for guggero November 15, 2023 02:06
@ProofOfKeags ProofOfKeags self-assigned this Nov 15, 2023
@saubyk saubyk added lncli documentation Documentation changes that do not affect code behaviour labels Nov 15, 2023
@saubyk saubyk added this to the v0.18.0 milestone Nov 15, 2023
Copy link
Collaborator

@ziggie1984 ziggie1984 left a comment

Choose a reason for hiding this comment

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

Checked the new commands and their corresponding docs, looks good. Thank you for fixing this issue.

Because we do not automate this check, I think it would make sense to add a remark in the Contribution checklist or somewhere else otherwise we will diverge again ?

.vscode/settings.json Show resolved Hide resolved
Copy link
Collaborator

@carlaKC carlaKC left a comment

Choose a reason for hiding this comment

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

Nice cleanup! Def agree that it needs contribution note (or gpt'ed up CI check 👀 ) to keep consistent.

Copy link
Collaborator

@ziggie1984 ziggie1984 left a comment

Choose a reason for hiding this comment

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

LGTM 🐝

"editor.rulers": [
80
],
"go.buildTags": "autopilotrpc chainrpc dev invoicesrpc neutrinorpc peersrpc signrpc walletrpc watchtowerrpc"
Copy link
Collaborator

Choose a reason for hiding this comment

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

Q: I wonder why you did not add the kv build tags for postgres or etcd ?

Copy link
Collaborator

@yyforyongyu yyforyongyu left a comment

Choose a reason for hiding this comment

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

Thanks for the PR!

@@ -16,7 +34,7 @@ service Router {
*/
rpc SendPaymentV2 (SendPaymentRequest) returns (stream lnrpc.Payment);
Copy link
Collaborator

Choose a reason for hiding this comment

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

should we also add this and all others? I'm assuming they are not added here because they are not referenced in the API documentation tool?

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

Correct. It's possible there is an existing incantation of lncli which uses this RPC but it wasn't obvious to me when I stepped through the various command services.

Copy link
Collaborator

@saubyk saubyk left a comment

Choose a reason for hiding this comment

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

Ack

@yyforyongyu yyforyongyu merged commit f005b24 into lightningnetwork:master Nov 21, 2023
24 of 25 checks passed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
documentation Documentation changes that do not affect code behaviour lncli
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Add corresponding lncli cmd tag in the protofiles so that is shows up in the API documentation
5 participants