Add CLI subcommands and Bash tab completion #600

hdwalters · 2024-11-16T17:44:26Z

Fixes #411 and #558.

Now specifies functionality via subcommands, with command-specific options:

Use amber eval to execute Amber code fragment.
Use amber run to execute Amber script.
Use amber check to check Amber script for errors.
Use amber build to compile Amber script to Bash.
Use amber docs to generate Amber script documentation.
Use amber comp to generate Bash completion script.

Now passes command line arguments to main block in Amber script.

hdwalters · 2024-11-16T19:08:17Z

src/compiler.rs

@@ -4,7 +4,7 @@ use crate::modules::block::Block;
 use crate::translate::check_all_blocks;
 use crate::translate::module::TranslateModule;
 use crate::utils::{ParserMetadata, TranslateMetadata};
-use crate::{rules, Cli};
+use crate::rules;


Cli is defined in main.rs, and would therefore be unavailable to a library crate, if we ever decide to separate the code into binary and library crates; removing references to it will help with that.

hdwalters · 2024-11-16T19:12:46Z

src/compiler.rs

+impl Default for CompilerOptions {
+    fn default() -> Self {
+        Self {
+            no_proc: vec![String::from("*")],


CompilerOptions is used everywhere AmberCompiler is used, which is to say almost everywhere. I do not think the --no-proc and --minify options make sense outside a cargo check or cargo build scenario, so we disable postprocessors for other subcommands. If anyone thinks we need to run postprocessors by default with cargo run, I can add a command line option for that.

i think it would make sense to add an option for run for disabling postprocessors, in case there are problems with it

My point is that postprocessors are currently enabled only for check and build, not for run subcommands. Your response implies that they should be enabled by default for run? (with an option to disable)

I don't use postprocessors at all. Moreover I think that for debugging purposes they should be disabled so that we can see what's actually being returned by amber compiler. Running them make sense when compiling amber in --release mode. Wdyt?

So right now the postprocessors, are a formatter and a a tool that inject code in the bash script.
I think that makes sense when amber is released that the postprocessor are enabled, but we have a tests if I am not wrong for those.

hdwalters · 2024-11-16T19:37:40Z

src/main.rs

-    #[arg(help = "'-' to read from stdin")]
+struct Cli {
+    #[command(subcommand)]
+    command: Option<CommandKind>,


Making command optional, and duplicating the input filename and args array at the Cli level, allows us to enter either amber run script.ab or amber script.ab. The latter is required to run scripts with a shebang line as ./script.ab.

src/main.rs

Ph0enixKM · 2024-11-17T08:23:50Z

Does the amber main.ab run the amber script too for shebang compatibility?

hdwalters · 2024-11-17T08:50:38Z

Does the amber main.ab run the amber script too for shebang compatibility?

It does:

hwalters@Ghostwheel ~/git/amber-lang (improve-command-line-interface) 
$ ./echo.ab 
Hello world
hwalters@Ghostwheel ~/git/amber-lang (improve-command-line-interface) 
$ amber echo.ab 
Hello world
hwalters@Ghostwheel ~/git/amber-lang (improve-command-line-interface) 
$ amber run echo.ab 
Hello world

Of course, this does mean users will have trouble with scripts called run, build etc. I think we can live with this.

Ph0enixKM · 2024-11-17T14:02:38Z

Thanks @hdwalters. That's great

hdwalters · 2024-11-17T21:11:33Z

Does anyone object if I fix #564 in this PR as well?

mks-h · 2024-11-18T00:24:34Z

I'm objecting to having amber run instead of the simple amber [path-to-file.ab] — that's going to make the shebang weird (#!/bin/amber run), and generally it would be less comfortable to use. Otherwise I don't see any problems.

P.S. If it isn't, the bash output filename in amber build should be optional, and by default be the same as of the Amber script, but having .sh ending.

hdwalters · 2024-11-18T07:47:19Z

I'm objecting to having amber run instead of the simple amber [path-to-file.ab] — that's going to make the shebang weird (#!/bin/amber run), and generally it would be less comfortable to use. Otherwise I don't see any problems.

@Ph0enixKM asked that exact same question; see my answer above. The revamped Cli struct has an input filename and command line arguments at both the top level and in the run subcommand, so either syntax works.

P.S. If it isn't, the bash output filename in amber build should be optional, and by default be the same as of the Amber script, but having .sh ending.

The output filename is already optional: the compiler creates a new filename by replacing .ab with .sh if not provided.

Mte90 · 2024-11-18T12:45:54Z

To me is fine if this PR fix also #564

b1ek · 2024-11-19T04:27:20Z

Does anyone object if I fix #564 in this PR as well?

is it even relevant? i guess its fine as long as it doesn't make a mess out of it. but change the title if you merge this PR with another one so it would be clear what is happening here

b1ek

lgtm so far, but it seems to be a draft for now (the #564) so i will review it later when its ready

b1ek · 2024-11-19T04:33:39Z

it also conflicts with #595

hdwalters · 2024-11-19T07:06:40Z

is it even relevant? i guess its fine as long as it doesn't make a mess out of it. but change the title if you merge this PR with another one so it would be clear what is happening here

It's relevant because for the first time, we can actually pass command line arguments directly to an Amber script, without compiling to Bash first. And given it's a one line change, I don't want to go through the pain of getting another PR approved! But since nobody has complained about doing it in the first place, I'm just going to push a fix.

lgtm so far, but it seems to be a draft for now (the #564) so i will review it later when its ready

It's not a draft, it's a feature proposal I haven't pushed yet.

it also conflicts with #595

Good grief. At 83 changed files, it would be hard for it not to conflict. But since I got mine in first, and yours is still a draft, it doesn't seem unreasonable to expect you to fix any merge conflicts!

src/modules/main.rs

b1ek · 2024-11-19T11:47:25Z

It's not a draft, it's a feature proposal I haven't pushed yet.

i meant that i can't tell my opinion if you haven't decided yet what is going to be changed in the final version

hdwalters · 2024-11-19T17:57:52Z

Ok. It's a moot point anyway, as I backed out the change.

Ph0enixKM

This was a long one but I got through it. Looking good! 3 small questions though.

src/utils/metadata/parser.rs

src/modules/function/declaration.rs

src/main.rs

Ph0enixKM · 2024-11-20T12:10:36Z

I just realized that the document module operates on files but it's a part of the compiler module. The file management part should be extracted to the main.rs. But that's not for the scope of this issue.

hdwalters · 2024-11-20T21:47:31Z

I just realized that the document module operates on files but it's a part of the compiler module. The file management part should be extracted to the main.rs. But that's not for the scope of this issue.

Another thing I would like to do at some point is refactor the entire codebase to use PathBuf and &Path for file system paths, instead of String and &str. I made the new CLI use the path specific structs at that level, but I had to convert to strings to pass into the compiler, to avoid feature creep.

Ph0enixKM

Looks good

hdwalters added 5 commits November 16, 2024 13:00

Minor aesthetic changes.

8407dcb

Parse arguments with subcommands.

fbea3c6

Pass arguments to Amber script.

fffefee

Optionally generate documentation with usage.

10e4623

Optionally generate Bash completion script.

51332c0

hdwalters requested a review from Ph0enixKM November 16, 2024 17:52

hdwalters commented Nov 16, 2024

View reviewed changes

hdwalters added 3 commits November 16, 2024 20:07

Write to stdout by default if building stdin.

5ee1e07

Reword help text for doc subcommand.

26fcfd8

Minor aesthetic changes.

ab79400

hdwalters requested a review from b1ek November 17, 2024 09:16

Change doc subcommand to docs.

fd8a69c

hdwalters requested a review from mks-h November 18, 2024 07:43

b1ek reviewed Nov 19, 2024

View reviewed changes

hdwalters changed the title ~~Improve command line interface~~ Implement subcommands, Bash tab completion, do not pass "$0" to main block Nov 19, 2024

hdwalters changed the title ~~Implement subcommands, Bash tab completion, do not pass "$0" to main block~~ Add subcommands, Bash tab completion, do not pass "$0" to main block Nov 19, 2024

hdwalters commented Nov 19, 2024

View reviewed changes

src/modules/main.rs Outdated Show resolved Hide resolved

hdwalters force-pushed the improve-command-line-interface branch from 42c86de to fd8a69c Compare November 19, 2024 08:19

hdwalters changed the title ~~Add subcommands, Bash tab completion, do not pass "$0" to main block~~ Add CLI subcommands and Bash tab completion Nov 19, 2024

b1ek approved these changes Nov 19, 2024

View reviewed changes

Ph0enixKM requested changes Nov 20, 2024

View reviewed changes

src/utils/metadata/parser.rs Show resolved Hide resolved

src/modules/function/declaration.rs Outdated Show resolved Hide resolved

src/main.rs Outdated Show resolved Hide resolved

hdwalters added 2 commits November 20, 2024 19:45

Change comp subcommand to completion.

de481d5

Rename helper function.

c3e0db3

hdwalters requested a review from Ph0enixKM November 20, 2024 20:10

hdwalters added 2 commits November 20, 2024 22:20

Enable postprocessors by default for run.

8309be8

Minor change to make Clippy happy.

649a76b

Ph0enixKM approved these changes Nov 21, 2024

View reviewed changes

Ph0enixKM requested a review from b1ek November 21, 2024 08:32

Mte90 approved these changes Nov 21, 2024

View reviewed changes

b1ek approved these changes Nov 21, 2024

View reviewed changes

hdwalters merged commit e5467d2 into amber-lang:master Nov 21, 2024
1 check passed

hdwalters deleted the improve-command-line-interface branch November 21, 2024 17:29

hdwalters mentioned this pull request Dec 3, 2024

[Feature] Add option to print Bash completion script #558

Closed

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Add CLI subcommands and Bash tab completion #600

Add CLI subcommands and Bash tab completion #600

hdwalters commented Nov 16, 2024 •

edited

Loading

hdwalters Nov 16, 2024

hdwalters Nov 16, 2024

b1ek Nov 20, 2024

hdwalters Nov 20, 2024 •

edited

Loading

Ph0enixKM Nov 20, 2024

Mte90 Nov 20, 2024

hdwalters Nov 16, 2024 •

edited

Loading

Ph0enixKM commented Nov 17, 2024

hdwalters commented Nov 17, 2024 •

edited

Loading

Ph0enixKM commented Nov 17, 2024

hdwalters commented Nov 17, 2024 •

edited

Loading

mks-h commented Nov 18, 2024

hdwalters commented Nov 18, 2024 •

edited

Loading

Mte90 commented Nov 18, 2024

b1ek commented Nov 19, 2024

b1ek left a comment

b1ek commented Nov 19, 2024

hdwalters commented Nov 19, 2024

b1ek commented Nov 19, 2024

hdwalters commented Nov 19, 2024

Ph0enixKM left a comment

Ph0enixKM commented Nov 20, 2024 •

edited

Loading

hdwalters commented Nov 20, 2024

Ph0enixKM left a comment

Add CLI subcommands and Bash tab completion #600

Add CLI subcommands and Bash tab completion #600

Conversation

hdwalters commented Nov 16, 2024 • edited Loading

hdwalters Nov 16, 2024

Choose a reason for hiding this comment

hdwalters Nov 16, 2024

Choose a reason for hiding this comment

b1ek Nov 20, 2024

Choose a reason for hiding this comment

hdwalters Nov 20, 2024 • edited Loading

Choose a reason for hiding this comment

Ph0enixKM Nov 20, 2024

Choose a reason for hiding this comment

Mte90 Nov 20, 2024

Choose a reason for hiding this comment

hdwalters Nov 16, 2024 • edited Loading

Choose a reason for hiding this comment

Ph0enixKM commented Nov 17, 2024

hdwalters commented Nov 17, 2024 • edited Loading

Ph0enixKM commented Nov 17, 2024

hdwalters commented Nov 17, 2024 • edited Loading

mks-h commented Nov 18, 2024

hdwalters commented Nov 18, 2024 • edited Loading

Mte90 commented Nov 18, 2024

b1ek commented Nov 19, 2024

b1ek left a comment

Choose a reason for hiding this comment

b1ek commented Nov 19, 2024

hdwalters commented Nov 19, 2024

b1ek commented Nov 19, 2024

hdwalters commented Nov 19, 2024

Ph0enixKM left a comment

Choose a reason for hiding this comment

Ph0enixKM commented Nov 20, 2024 • edited Loading

hdwalters commented Nov 20, 2024

Ph0enixKM left a comment

Choose a reason for hiding this comment

hdwalters commented Nov 16, 2024 •

edited

Loading

hdwalters Nov 20, 2024 •

edited

Loading

hdwalters Nov 16, 2024 •

edited

Loading

hdwalters commented Nov 17, 2024 •

edited

Loading

hdwalters commented Nov 17, 2024 •

edited

Loading

hdwalters commented Nov 18, 2024 •

edited

Loading

Ph0enixKM commented Nov 20, 2024 •

edited

Loading