Implement standard library glob function #511

hdwalters · 2024-10-11T21:53:04Z

Fixes #463, by implementing glob as a standard library function. I also fixed a bug where failing functions would return the previous array value with the first item blanked out, because the default return value was Bash string '' not Bash array ().

hdwalters · 2024-10-11T22:01:40Z

src/modules/condition/failed.rs

@@ -80,23 +80,27 @@ impl TranslateModule for Failed {
            if self.is_question_mark {
                // if the failed expression is in the main block we need to clear the return value
                let clear_return = if !self.is_main {
-                    let (name, id, variant) = meta.fun_name.clone().expect("Function name not set");
-                    format!("__AF_{name}{id}_v{variant}=''")
+                    let fun_name = meta.fun_name.as_ref().expect("Function name not set");


Bundled (name, id, variant) into a new struct FunctionName, because we were duplicating mangled name generation in three places.

hdwalters · 2024-10-11T22:05:42Z

src/modules/condition/failed.rs

-                    let (name, id, variant) = meta.fun_name.clone().expect("Function name not set");
-                    format!("__AF_{name}{id}_v{variant}=''")
+                    let fun_name = meta.fun_name.as_ref().expect("Function name not set");
+                    format!("{}={}", fun_name.mangled_name(), fun_name.default_value())


This fixes a bug where failing functions would return the previous array value with the first item blanked out, because the default return value was Bash string '' not Bash array (). Try running the following Amber script with commits up to "5212ef5 Add glob functions to standard library":

hwalters@Ghostwheel ~/git/amber (HEAD) $ cat glob.ab #!/usr/bin/env amber import * from "std/fs" echo "Calling glob(\"*.md\")" loop file in trust glob("*.md") { echo "Found \"{file}\"" } echo "Calling glob(\"*.xyz\")" loop file in trust glob("*.xyz") { echo "Found \"{file}\"" }

hwalters@Ghostwheel ~/git/amber (HEAD) $ ./glob.ab Calling glob("*.md") Found "CONTRIBUTING.md" Found "LICENSE.md" Found "NIX.md" Found "README.md" Calling glob("*.xyz") Found "" Found "LICENSE.md" Found "NIX.md" Found "README.md"

This is what it does with the latest commit:

hwalters@Ghostwheel ~/git/amber (implement-stdlib-glob-function) $ ./glob.ab Calling glob("*.md") Found "CONTRIBUTING.md" Found "LICENSE.md" Found "NIX.md" Found "README.md" Calling glob("*.xyz")

hdwalters · 2024-10-11T22:13:04Z

src/modules/types.rs

@@ -30,6 +30,14 @@ impl Type {
    pub fn is_allowed_in(&self, other: &Type) -> bool {
        self == other || self.is_subset_of(other)
    }
+
+    pub fn is_array(&self) -> bool {


This may cause problems if the union types change supports union types in return values. If a function could return Text | [Text] (which seems like something we would want to disallow, tbh) what would the default return type be?

Then the default can be the first type of the union, Text in your case. No need to disallow this, it's one of union's use cases.

hdwalters · 2024-10-11T22:19:03Z

src/std/fs.ab

+///     }
+/// }
+/// ```
+pub fun glob_multiple(path: [Text]): [Text]? {


Seems like a good idea to eventually support either glob("*.txt") or glob(["*.txt", "*.xyz"]). This function is here as a placeholder for that functionality, so we can write unit tests. To be merged into the main function when the union types PR is merged.

hdwalters · 2024-10-11T22:21:18Z

src/tests/stdlib/create_symbolic_link.ab

@@ -7,5 +7,5 @@ main {
    } else {
        echo "failed"
    }
-    trust $rm {tmpdir}$
+    trust $rm -fr {tmpdir}$


I believe this is also part of a different pending PR. Shouldn't cause merge conflicts, but this is implemented in a commit of its own, so I can drop it and rebase if it causes problems.

mks-h

Generally speaking this is a very good looking PR. Just a few nitpicks, and it would be good to safeguard the escaping logic with some tests.

src/std/fs.ab

mks-h · 2024-10-12T02:55:28Z

src/modules/types.rs

@@ -30,6 +30,14 @@ impl Type {
    pub fn is_allowed_in(&self, other: &Type) -> bool {
        self == other || self.is_subset_of(other)
    }
+
+    pub fn is_array(&self) -> bool {


Then the default can be the first type of the union, Text in your case. No need to disallow this, it's one of union's use cases.

src/utils/function_name.rs

src/utils/metadata/translate.rs

src/utils/function_name.rs

src/modules/function/ret.rs

src/std/fs.ab

b1ek · 2024-10-15T06:57:04Z

this is very confusing to review.

are you fixing an underlying issue right away which blocks you from implementing the function in stdlib? then it should be in two different PR's, with this one depending on the one that resolves the issue in the compiler

also the default return thing is a new language feature, it should be a feature request issue first, and then a PR

hdwalters · 2024-10-15T17:10:24Z

this is very confusing to review.

are you fixing an underlying issue right away which blocks you from implementing the function in stdlib? then it should be in two different PR's, with this one depending on the one that resolves the issue in the compiler

also the default return thing is a new language feature, it should be a feature request issue first, and then a PR

To summarise our private discussion, I'll create a new issue and PR, to fix the "default return" problem, then rebase this PR once that's been merged. But in return, you have to review both of them! (grin)

hdwalters · 2024-10-15T17:33:26Z

Created issue #515.

hdwalters · 2024-10-15T19:58:38Z

Created issue #515.

And created pull request #516 to fix that; have added @b1ek and @mks-h as reviewers. I'll rebase this PR branch once that's been merged.

src/std/fs.ab

hdwalters requested review from Mte90, Ph0enixKM, mks-h, b1ek and KrosFire October 11, 2024 21:53

hdwalters commented Oct 11, 2024

View reviewed changes

mks-h requested changes Oct 12, 2024

View reviewed changes

hdwalters added bug Something isn't working enhancement New feature or request and removed bug Something isn't working labels Oct 12, 2024

hdwalters self-assigned this Oct 12, 2024

hdwalters requested a review from mks-h October 12, 2024 10:19

mks-h reviewed Oct 12, 2024

View reviewed changes

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

hdwalters requested a review from mks-h October 12, 2024 12:41

mks-h reviewed Oct 12, 2024

View reviewed changes

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

hdwalters requested a review from mks-h October 12, 2024 18:24

mks-h requested changes Oct 13, 2024

View reviewed changes

src/std/fs.ab Outdated Show resolved Hide resolved

src/std/fs.ab Outdated Show resolved Hide resolved

src/std/fs.ab Outdated Show resolved Hide resolved

mks-h reviewed Oct 13, 2024

View reviewed changes

src/std/fs.ab Outdated Show resolved Hide resolved

src/std/fs.ab Outdated Show resolved Hide resolved

src/std/fs.ab Outdated Show resolved Hide resolved

hdwalters requested a review from mks-h October 13, 2024 09:37

mks-h reviewed Oct 14, 2024

View reviewed changes

src/std/fs.ab Outdated Show resolved Hide resolved

hdwalters requested a review from mks-h October 14, 2024 17:22

hdwalters added 2 commits October 19, 2024 07:06

Reinstate command to remove /tmp directory.

ce16ee8

Add glob functions to standard library.

d9569af

hdwalters force-pushed the implement-stdlib-glob-function branch from 5dc0e08 to d9569af Compare October 19, 2024 06:06

b1ek approved these changes Oct 19, 2024

View reviewed changes

Whitespace changes.

984e7fe

Mte90 approved these changes Oct 19, 2024

View reviewed changes

mks-h approved these changes Oct 21, 2024

View reviewed changes

src/std/fs.ab Outdated Show resolved Hide resolved

src/std/fs.ab Outdated Show resolved Hide resolved

src/std/fs.ab Show resolved Hide resolved

Reinstate removed blank lines.

54b2bcb

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Implement standard library glob function #511

Implement standard library glob function #511

hdwalters commented Oct 11, 2024

hdwalters Oct 11, 2024

hdwalters Oct 11, 2024

hdwalters Oct 11, 2024

mks-h Oct 12, 2024

hdwalters Oct 11, 2024

hdwalters Oct 11, 2024

mks-h left a comment

mks-h Oct 12, 2024

b1ek commented Oct 15, 2024

hdwalters commented Oct 15, 2024

hdwalters commented Oct 15, 2024

hdwalters commented Oct 15, 2024

Implement standard library glob function #511

Are you sure you want to change the base?

Implement standard library glob function #511

Conversation

hdwalters commented Oct 11, 2024

hdwalters Oct 11, 2024

Choose a reason for hiding this comment

hdwalters Oct 11, 2024

Choose a reason for hiding this comment

hdwalters Oct 11, 2024

Choose a reason for hiding this comment

mks-h Oct 12, 2024

Choose a reason for hiding this comment

hdwalters Oct 11, 2024

Choose a reason for hiding this comment

hdwalters Oct 11, 2024

Choose a reason for hiding this comment

mks-h left a comment

Choose a reason for hiding this comment

mks-h Oct 12, 2024

Choose a reason for hiding this comment

b1ek commented Oct 15, 2024

hdwalters commented Oct 15, 2024

hdwalters commented Oct 15, 2024

hdwalters commented Oct 15, 2024