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

server: use a command line parser #33

Merged
merged 2 commits into from
Dec 11, 2024

Conversation

vincenzopalazzo
Copy link
Contributor

This commit adds a command line parser to the server. The parser is useful to avoid crashing when trying to run a simple --help command and also in the future when we will add more configuration options.

It will be really cool if the config file is optional and the user can configure the server using the command line by appending all the options to ldk-server --option1 value1 --option2 value2.

     Running `target/debug/ldk-server --help`
thread 'main' panicked at ldk-server/src/main.rs:30:56:
Invalid configuration file.: Custom { kind: NotFound, error: "Failed to read config file '--help': No such file or directory (os error 2)" }
note: run with `RUST_BACKTRACE=1` environment variable to display a backtrace
[1]    750067 IOT instruction (core dumped)  cargo run --bin ldk-server -- --help

@vincenzopalazzo vincenzopalazzo force-pushed the macros/fix-help-command branch 2 times, most recently from 45a6504 to 6c47ec0 Compare December 7, 2024 00:30
@vincenzopalazzo
Copy link
Contributor Author

I do not know if you want to keep the configuration a positional argument or it is fine introduce a --config flag

@vincenzopalazzo vincenzopalazzo force-pushed the macros/fix-help-command branch from 6c47ec0 to 9a240f8 Compare December 7, 2024 00:31
ldk-server/Cargo.toml Outdated Show resolved Hide resolved
@G8XSU
Copy link
Contributor

G8XSU commented Dec 7, 2024

Currently we avoid clap dependency in server since we need a single argument.
We don't foresee the number of args increasing. Everything else should just be part of config.

@vincenzopalazzo
Copy link
Contributor Author

Currently we avoid clap dependency in server since we need a single argument. We don't foresee the number of args increasing. Everything else should just be part of config.

Oh I see make a lot of sense, so I will update the PR in according with this! sorry for the miss understanding!

@vincenzopalazzo
Copy link
Contributor Author

vincenzopalazzo commented Dec 10, 2024

Ok sorry for the delay here.

I modified the code and just make a small match on the --help string. I updated the commit description too 57959aa probably I will not update the PR description otherwise the discussion did not make anymore sense for people in the future?

P.S: I rebase the PR on top of the @dzdidi PR!

ldk-server/src/main.rs Outdated Show resolved Hide resolved
ldk-server/src/main.rs Show resolved Hide resolved
ldk-server/src/main.rs Outdated Show resolved Hide resolved
ldk-server/src/main.rs Outdated Show resolved Hide resolved
@vincenzopalazzo
Copy link
Contributor Author

While reading your suggestion on some review @G8XSU I wrote another commit to check if the config file is a file f3cee47

let me know if you prefer this solution and remove the --help, or if you want to keep both. Anything is fine for me, I preferer keep --help because I use a lot of command line and for me (and I guess many command line users) the --help is the main source of docs 😸

ldk-server/src/main.rs Outdated Show resolved Hide resolved
ldk-server/src/main.rs Outdated Show resolved Hide resolved
This commit is including a simple check and suggest a help string
for the user when the command `--help` is used. Without this diff
applied the server will panic and the user will not have any clue
why.

     Running `target/debug/ldk-server --help`
thread 'main' panicked at ldk-server/src/main.rs:30:56:
Invalid configuration file.: Custom { kind: NotFound, error: "Failed to read config file '--help': No such file or directory (os error 2)" }
note: run with `RUST_BACKTRACE=1` environment variable to display a backtrace
[1]    750067 IOT instruction (core dumped)  cargo run --bin ldk-server -- --help

Signed-off-by: Vincenzo Palazzo <[email protected]>
@G8XSU
Copy link
Contributor

G8XSU commented Dec 11, 2024

Lgtm! Thanks !

@G8XSU G8XSU merged commit 40f6bcd into lightningdevkit:main Dec 11, 2024
6 checks passed
@vincenzopalazzo vincenzopalazzo deleted the macros/fix-help-command branch December 11, 2024 11:50
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants