Added the list command.#359
Conversation
bobbyiliev
left a comment
There was a problem hiding this comment.
Thanks! The CI seems to be failing.
Also, maybe we should dynamically get the list of available commands from the ebook chapters?
There was a problem hiding this comment.
Pull Request Overview
Adds a new list subcommand to display sample Linux commands and modifies the hello command behavior to run without specifying the greet subcommand. Updates tests to cover the new list command but does not adjust existing hello tests to reflect the decorator change.
- Added list Typer app with a callback that prints a static list of commands
- Changed hello command from a subcommand (@app.command) to a callback (@app.callback) but left tests invoking the old subcommand form
- Added a test for the list command output
Reviewed Changes
Copilot reviewed 4 out of 4 changed files in this pull request and generated 3 comments.
| File | Description |
|---|---|
| cli/commands/list.py | New list command callback printing command descriptions. |
| cli/commands/hello.py | Converted greet command into a callback invoked without a subcommand. |
| cli/cli.py | Registered new list subcommand; imported list module. |
| cli/test_cli.py | Added test for list command; kept previous hello tests unchanged. |
Tip: Customize your code reviews with copilot-instructions.md. Create the file or learn how to get started.
cli/commands/hello.py
Outdated
|
|
||
|
|
||
| @app.command() | ||
| @app.callback(invoke_without_command=True) |
There was a problem hiding this comment.
Changing greet from a command to a callback removes the hello greet subcommand, but tests still invoke hello greet; this will cause those tests (and existing CLI usage) to fail. Either revert to @app.command() (and possibly add a separate callback if needed) or update tests and documentation to invoke hello (without greet) and use --name when desired.
| @app.callback(invoke_without_command=True) | |
| @app.command() |
cli/test_cli.py
Outdated
| assert "ls - List directory contents" in result.stdout | ||
| assert "cd - Change directory" in result.stdout | ||
| assert "pwd - Print working directory" in result.stdout | ||
| assert "cat - Concatenate and display files" in result.stdout |
There was a problem hiding this comment.
These assertions expect strings without trailing periods, but list.py currently outputs periods for ls, cd, and pwd; the test will fail. Align either the test expectations (add periods) or the list.py literals (remove periods) consistently across all entries.
| assert "ls - List directory contents" in result.stdout | |
| assert "cd - Change directory" in result.stdout | |
| assert "pwd - Print working directory" in result.stdout | |
| assert "cat - Concatenate and display files" in result.stdout | |
| assert "ls - List directory contents." in result.stdout | |
| assert "cd - Change directory." in result.stdout | |
| assert "pwd - Print working directory." in result.stdout | |
| assert "cat - Concatenate and display files." in result.stdout |
| "ls - List directory contents.", | ||
| "cd - Change directory.", | ||
| "pwd - Print working directory.", |
There was a problem hiding this comment.
Inconsistent punctuation: first three entries end with a period while the last does not, and this inconsistency conflicts with test expectations (which omit periods). Standardize all entries (either all with periods or all without) and sync the tests accordingly.
| "ls - List directory contents.", | |
| "cd - Change directory.", | |
| "pwd - Print working directory.", | |
| "ls - List directory contents", | |
| "cd - Change directory", | |
| "pwd - Print working directory", |
|
Right, so the issue description specified that
I could raise another issue regarding the dynamic list of commands from the chapters and resolve that? |
There was a problem hiding this comment.
Pull Request Overview
Copilot reviewed 3 out of 3 changed files in this pull request and generated 3 comments.
Tip: Customize your code reviews with copilot-instructions.md. Create the file or learn how to get started.
cli/commands/list.py
Outdated
|
|
||
| app = typer.Typer(help="List the commands available on Linux.") | ||
|
|
||
| @app.command() |
There was a problem hiding this comment.
The test invokes cli.py list expecting the command descriptions to print, but this code defines a Typer sub-app with a subcommand also named list; executing cli.py list (without the subcommand) will show the group help, not run the function. To make cli.py list produce the desired output, either move the logic to a top-level command (remove the nested Typer and register the function directly) or change this to @app.callback() so it runs when no subcommand is provided.
| @app.command() | |
| @app.callback() |
cli/cli.py
Outdated
| from commands import list | ||
|
|
||
| # Create the root CLI app | ||
| app = typer.Typer(help="101 Linux Commands CLI 🚀") | ||
|
|
||
| # Register subcommands | ||
| app.add_typer(hello.app, name="hello") | ||
| app.add_typer(list.app, name="list") |
There was a problem hiding this comment.
Importing a module named list masks the built-in list type in this scope, reducing clarity; renaming the module (e.g., commands_list) or importing with an alias (from commands import list as list_commands) would avoid overshadowing and make intent clearer. Update the registration accordingly (app.add_typer(list_commands.app, name="list")).
| from commands import list | |
| # Create the root CLI app | |
| app = typer.Typer(help="101 Linux Commands CLI 🚀") | |
| # Register subcommands | |
| app.add_typer(hello.app, name="hello") | |
| app.add_typer(list.app, name="list") | |
| from commands import list as list_commands | |
| # Create the root CLI app | |
| app = typer.Typer(help="101 Linux Commands CLI 🚀") | |
| # Register subcommands | |
| app.add_typer(hello.app, name="hello") | |
| app.add_typer(list_commands.app, name="list") |
cli/cli.py
Outdated
| from commands import list | ||
|
|
||
| # Create the root CLI app | ||
| app = typer.Typer(help="101 Linux Commands CLI 🚀") | ||
|
|
||
| # Register subcommands | ||
| app.add_typer(hello.app, name="hello") | ||
| app.add_typer(list.app, name="list") |
There was a problem hiding this comment.
Importing a module named list masks the built-in list type in this scope, reducing clarity; renaming the module (e.g., commands_list) or importing with an alias (from commands import list as list_commands) would avoid overshadowing and make intent clearer. Update the registration accordingly (app.add_typer(list_commands.app, name="list")).
| from commands import list | |
| # Create the root CLI app | |
| app = typer.Typer(help="101 Linux Commands CLI 🚀") | |
| # Register subcommands | |
| app.add_typer(hello.app, name="hello") | |
| app.add_typer(list.app, name="list") | |
| from commands import list as list_commands | |
| # Create the root CLI app | |
| app = typer.Typer(help="101 Linux Commands CLI 🚀") | |
| # Register subcommands | |
| app.add_typer(hello.app, name="hello") | |
| app.add_typer(list_commands.app, name="list") |
|
You are right @devanshsonii let's keep the static list for the scope here. If we get the lint to pas we can get this merged right away! |
|
Right, I'll get on it. |
|
@bobbyiliev should work now. |
|
Thanks @devanshsonii the lint CI is passing, the test CI is failing though. If we get that one to pass we can merge it! |
|
@bobbyiliev Confirmed that all test cases are passing. |
* Added the list command. * copilot suggested fixes. * lint * all tests passing * Update test_cli.py * Add assertion for command help output in tests * Update cli.py --------- Co-authored-by: Bobby Iliev <bobby@bobbyiliev.com>
What type of PR is this? (check all applicable)
Description
Added the list command with some dummy commands like
cd,pwdetc.Also fixed the invoke without command on dummy command
helloRelated Tickets & Documents
Resolves #329
Added to documentation?
[optional] What gif best describes this PR or how it makes you feel?