Alias builtin by CraigNewlands · Pull Request #44 · siliconlad/aish

CraigNewlands · 2024-07-25T21:01:39Z

No description provided.

CraigNewlands · 2024-07-25T21:02:40Z

works without quotes but will test with quotes and with spaces once we have sported the parser to handle the speech marks

src/main.rs

siliconlad · 2024-07-25T21:09:40Z

This you might need to revise/force push to fix the merge conflicts too. Other than that (and my other comment) looks good!

siliconlad

I rebased this branch so fix the diffs.

src/main.rs

siliconlad · 2024-07-30T17:10:06Z

I've merged most of the other PRs so we should be able to revert the temp fix.

CraigNewlands · 2024-08-07T10:32:43Z

src/builtins.rs

siliconlad

Sorry, just realized that we probably want to support alias substitution in slightly more complicated scenarios haha 😅

src/builtins.rs

CraigNewlands · 2024-09-08T10:57:08Z

siliconlad · 2024-09-08T11:03:58Z

Should the "Hello world" have double quotes when greet is run?

CraigNewlands · 2024-09-08T11:11:48Z

Should the "Hello world" have double quotes when greet is run?

It should not hahah, will have a look at that

CraigNewlands · 2024-09-17T19:12:01Z

After 2 months i think its finally done hahaha

siliconlad

A couple of comments :)

src/main.rs

src/builtins.rs

src/main.rs

src/builtins.rs

src/command.rs

CraigNewlands · 2024-09-28T12:19:40Z

Completely changed my approach so now expanding the aliases before they are tokenized. This seems much simpler and means the string is still tokenized properly and we don't need to retokenize in the alias expansion if we did it after the tokenization. Seems to work for all the tests I've done - using piping and recursive aliases etc. What do you think?

CraigNewlands · 2024-09-28T12:33:29Z

Found a couple more things to fix

CraigNewlands · 2024-09-28T12:33:52Z

So many edge cases 😂

siliconlad · 2024-09-28T16:31:45Z

I think we should merge #82 so we can add all the edge-cases as tests.

CraigNewlands · 2024-09-29T11:01:29Z

I think we should merge #82 so we can add all the edge-cases as tests.

Merged, will add some tests now for most of the edge cases I can think of.

CraigNewlands · 2024-09-29T14:20:34Z

@siliconlad can you think of a way I can get around this problem I'm having...

Currently if we do

> alias greet='echo "Hello, World!"'
> greet
Hello, World!
> alias greet='echo "Hello, World!"' && alias
greet='echo "Hello, World!"'
> alias greet='echo "Hello, World!"' && greet
No such file or directory (os error 2)
>

the last line doesn't work because expand_aliases is called before we tokenize and then add alias greet='echo "Hello, World!"' to the aliases hashmap, which is done in tokenize.run, so it is not expanded. Can you think of a good way of doing this?

siliconlad · 2024-09-29T14:42:42Z

What I might try (in expand_aliases) is the following sequence of steps:

Split string by ;
Split string by &&
Split string by |
Replace the first word of each chunk by an alias if available

Would that work? Maybe you also need to deal with aliases using aliases. Perhaps that's just repeated application of the steps above until you detect no more aliases (might have to detect loops / set maximum alias depth).

CraigNewlands · 2024-10-07T22:52:15Z

It's not the prettiest code in the world but functionally I think it finally works. Going to try make it a bit nicer to read, especially the expand_aliases function lol

CraigNewlands · 2024-10-07T23:01:40Z

@siliconlad think we're good to go on this PR. Ready for review, hopefully for the final time lol

siliconlad

I guess the main thing is it works.

I left some comments mainly about what bash does and whether we want to behave the same way or not. I think some of it will allow us to cut down on the complexity that alias adds (e.g. in pipe). What do you think?

siliconlad · 2024-10-08T16:27:50Z

src/builtins.rs

+            }
+        }
+    } else {
+        Err("Usage: alias [name[=value] ...]".into())


What's the ...?

siliconlad · 2024-10-08T16:35:42Z

src/command.rs

-    fn pipe(&self, stdin: Option<ChildStdout>) -> Result<Option<ChildStdout>, Box<dyn Error>> {
+    fn pipe(
+        &self,
+        stdin: Option<ChildStdout>,
+        aliases: &mut HashMap<String, String>,
+    ) -> Result<Option<ChildStdout>, Box<dyn Error>> {
        let (pipe_out_r, pipe_out_w) = pipe()?;
        let (pipe_err_r, pipe_err_w) = pipe()?;
+        let (alias_r, alias_w) = pipe()?;

        match unsafe { fork() }? {
            ForkResult::Parent { child: _ } => {
                drop(pipe_out_w);
                drop(pipe_err_w);
+                drop(alias_w);
+
+                // Read updated aliases from child
+                let mut alias_reader = unsafe { File::from_raw_fd(alias_r.into_raw_fd()) };
+                let mut serialized_aliases = String::new();
+                alias_reader.read_to_string(&mut serialized_aliases)?;
+                if !serialized_aliases.is_empty() {
+                    *aliases = serde_json::from_str(&serialized_aliases)?;
+                }
+
                Ok(Some(ChildStdout::from(pipe_out_r)))
            }
            ForkResult::Child => {
                drop(pipe_out_r);
                drop(pipe_err_r);
+                drop(alias_r);
+
                dup2(pipe_out_w.as_raw_fd(), 1)?;
                dup2(pipe_err_w.as_raw_fd(), 2)?;
-                self.run_builtin(stdin)?;
+                self.run_builtin(stdin, aliases)?;
+
+                // Send updated aliases to parent
+                let serialized_aliases = serde_json::to_string(aliases)?;
+                let mut alias_writer = unsafe { File::from_raw_fd(alias_w.into_raw_fd()) };
+                write!(alias_writer, "{}", serialized_aliases)?;
+


This is really cool haha. I'm assuming that this is for this particular scenario:

> alias r='echo' | r Hello Hello

But this doesn't work in Bash, so we don't necessarily need to support it? Might cut down on the complexity.

siliconlad · 2024-10-08T16:39:32Z

src/main.rs

+fn handle_alias_definition(temp_aliases: &mut HashMap<String, String>, args: &[String]) {
+    let full_command = args.join(" ");
+    if let Some(equals_pos) = full_command.find('=') {
+        let (alias_name, alias_value) = full_command.split_at(equals_pos);
+        temp_aliases.insert(
+            alias_name.to_string(),
+            alias_value[1..].trim_matches('\'').to_string(),
+        );
+    }
+}


What is this for?

It's solving the problem where we have something like alias greet='echo \"Hello, World!\"' && greet because expand tokens gets called before we add greet to the aliases, the second greet after the && doesn't get expanded so this checks if we have an alias on the left of the command and if we do then it expands the right hand side. So basically all handle_alias_definition is doing is getting the alias greet and the expansion of this which is echo "Hello, World!", this is then returned and used in expand_command_aliases to turn the whole command into alias greet='echo \"Hello, World!\"' && alias greet='echo \"Hello, World!\"

This could probably be made into one function as we are doing the same thing in the alias buitlin command.

siliconlad · 2024-10-08T16:49:22Z

tests/integration.rs

I guess it doesn't hurt to have all these tests, but probably worth having tests that execute aliases (you should be able to use ;).

siliconlad · 2024-10-08T16:49:48Z

src/main.rs

+    while let Some(expansion) = temp_aliases.get(&words[0]) {
+        let expanded_words: Vec<String> = expansion.split_whitespace().map(String::from).collect();
+        if expanded_words.is_empty() || expansion_count >= MAX_EXPANSIONS {
+            break;
+        }
+        words.splice(0..1, expanded_words);
+        expansion_count += 1;
+    }


Reading the reference manual, they actually don't do recursive expansions.

siliconlad · 2024-10-08T16:50:27Z

src/builtins.rs

+        if parts.len() == 2 {
+            let alias = parts[0];
+            let command = parts[1];
+            aliases.insert(alias.to_string(), command.to_string());


The reference manual does not allow special characters in the name. Not sure if we want to enforce that or not :)

CraigNewlands linked an issue Jul 25, 2024 that may be closed by this pull request

alias builtin #40

Open

siliconlad reviewed Jul 25, 2024

View reviewed changes

src/main.rs Outdated Show resolved Hide resolved

siliconlad force-pushed the 40-alias-builtin branch from cc40b84 to 25e41b6 Compare July 29, 2024 16:34

siliconlad reviewed Jul 29, 2024

View reviewed changes

src/main.rs Outdated Show resolved Hide resolved

siliconlad changed the title ~~40 alias builtin~~ Alias builtin Jul 29, 2024

CraigNewlands force-pushed the 40-alias-builtin branch from e74a9ce to fb25ae4 Compare August 7, 2024 10:22

CraigNewlands commented Aug 8, 2024

View reviewed changes

src/builtins.rs Outdated Show resolved Hide resolved

siliconlad assigned CraigNewlands Aug 17, 2024

CraigNewlands force-pushed the 40-alias-builtin branch from fc0a41c to 3ad19ea Compare August 29, 2024 21:49

CraigNewlands commented Aug 29, 2024

View reviewed changes

src/builtins.rs Outdated Show resolved Hide resolved

siliconlad requested changes Aug 30, 2024

View reviewed changes

src/builtins.rs Outdated Show resolved Hide resolved

src/builtins.rs Outdated Show resolved Hide resolved

CraigNewlands force-pushed the 40-alias-builtin branch from 63ffbf0 to 2f702b3 Compare September 8, 2024 10:56

CraigNewlands force-pushed the 40-alias-builtin branch from 2f702b3 to ac3902b Compare September 8, 2024 11:09

CraigNewlands force-pushed the 40-alias-builtin branch 3 times, most recently from 1efd75c to 31c6bbd Compare September 17, 2024 19:09

CraigNewlands requested a review from siliconlad September 17, 2024 19:12

siliconlad requested changes Sep 18, 2024

View reviewed changes

src/main.rs Outdated Show resolved Hide resolved

src/builtins.rs Outdated Show resolved Hide resolved

src/main.rs Outdated Show resolved Hide resolved

src/builtins.rs Outdated Show resolved Hide resolved

siliconlad reviewed Sep 19, 2024

View reviewed changes

src/command.rs Outdated Show resolved Hide resolved

CraigNewlands force-pushed the 40-alias-builtin branch 3 times, most recently from f9e5d05 to fe785f4 Compare September 28, 2024 12:14

CraigNewlands force-pushed the 40-alias-builtin branch 2 times, most recently from 363e6f7 to 4c48cc0 Compare September 28, 2024 12:27

CraigNewlands force-pushed the 40-alias-builtin branch from bdfbe22 to 3a37646 Compare September 28, 2024 12:39

CraigNewlands requested a review from siliconlad September 29, 2024 10:59

CraigNewlands force-pushed the 40-alias-builtin branch 2 times, most recently from fcf49f4 to d4a320f Compare September 29, 2024 14:15

CraigNewlands force-pushed the 40-alias-builtin branch from d4a320f to f8ebaa5 Compare September 29, 2024 14:23

CraigNewlands closed this Oct 7, 2024

CraigNewlands force-pushed the 40-alias-builtin branch from f8ebaa5 to 99cfa47 Compare October 7, 2024 22:43

CraigNewlands reopened this Oct 7, 2024

CraigNewlands force-pushed the 40-alias-builtin branch from 2d3280c to e0c9dd8 Compare October 7, 2024 22:50

CraigNewlands force-pushed the 40-alias-builtin branch 2 times, most recently from 8ffd384 to 4ac43fe Compare October 7, 2024 22:58

siliconlad reviewed Oct 8, 2024

View reviewed changes

added the alias command and tests

9552fac

CraigNewlands force-pushed the 40-alias-builtin branch from 4ac43fe to 9552fac Compare October 16, 2024 21:42

Conversation

CraigNewlands commented Jul 25, 2024

Uh oh!

CraigNewlands commented Jul 25, 2024

Uh oh!

Uh oh!

siliconlad commented Jul 25, 2024

Uh oh!

siliconlad left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

siliconlad commented Jul 30, 2024

Uh oh!

CraigNewlands commented Aug 7, 2024

Uh oh!

Uh oh!

Uh oh!

siliconlad left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

CraigNewlands commented Sep 8, 2024

Uh oh!

siliconlad commented Sep 8, 2024

Uh oh!

CraigNewlands commented Sep 8, 2024

Uh oh!

CraigNewlands commented Sep 17, 2024

Uh oh!

siliconlad left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

CraigNewlands commented Sep 28, 2024

Uh oh!

CraigNewlands commented Sep 28, 2024

Uh oh!

CraigNewlands commented Sep 28, 2024

Uh oh!

siliconlad commented Sep 28, 2024

Uh oh!

CraigNewlands commented Sep 29, 2024

Uh oh!

CraigNewlands commented Sep 29, 2024

Uh oh!

siliconlad commented Sep 29, 2024 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

CraigNewlands commented Oct 7, 2024

Uh oh!

CraigNewlands commented Oct 7, 2024

Uh oh!

siliconlad left a comment

Choose a reason for hiding this comment

Uh oh!

siliconlad Oct 8, 2024

Choose a reason for hiding this comment

Uh oh!

siliconlad Oct 8, 2024

Choose a reason for hiding this comment

Uh oh!

siliconlad Oct 8, 2024

Choose a reason for hiding this comment

Uh oh!

CraigNewlands Oct 16, 2024

Choose a reason for hiding this comment

Uh oh!

CraigNewlands Oct 16, 2024 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

siliconlad Oct 8, 2024

Choose a reason for hiding this comment

Uh oh!

siliconlad commented Sep 29, 2024 •

edited

Loading

CraigNewlands Oct 16, 2024 •

edited

Loading