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

feat(std): more commands #185

Merged
merged 23 commits into from
Jul 1, 2024
Merged

feat(std): more commands #185

merged 23 commits into from
Jul 1, 2024

Conversation

Mte90
Copy link
Member

@Mte90 Mte90 commented Jun 12, 2024

So I added more commands:

  • move a wrapper around mv
  • change_dir a wrapper around cd but in this way it is generated a cd command fine for shellcheck [Feature] Integration with shellcheck #72
  • make_symbolic_link a wrapper around ln -s
  • create_dir a wrapper around mkdir -p
  • make_executable a wrapper around chmod +x
  • switch_user_permission a wrapper around chown -R
  • delete a wrapper around rm -fr

In many of those cases there is a check if the file/folder exist, in case return a different boolean value and print the error.
In 2 cases we need the OR condition so we can check if it is file and a folder.

@b1ek
Copy link
Member

b1ek commented Jun 13, 2024

cd could (and probably should) be added as a bulit in command to amber, like echo

@Ph0enixKM
Copy link
Member

Ph0enixKM commented Jun 13, 2024

@b1ek all the non-failable GNU commands that are parto of POSIX should be built-in.

@b1ek
Copy link
Member

b1ek commented Jun 13, 2024

@b1ek all the non-failable GNU commands that are parto of POSIX should be built-in.

and most of these

@Ph0enixKM
Copy link
Member

Actually... perhaps built-ins could be failable as well:

mv "path/to/file.txt" "other/path/to/file.txt" failed {
    echo "Couldn't move the file"
}

@Mte90
Copy link
Member Author

Mte90 commented Jun 13, 2024

Let's do a list of commands that need a native support and what needs a function.
Maybe we can support them natively and create functions like I did that improve the usage like a normal scripting language.

@Ph0enixKM Ph0enixKM added this to the Amber 0.4.0-alpha milestone Jun 17, 2024
@s3than
Copy link

s3than commented Jun 18, 2024

This function should have the following changes

pub fun make_executable(path: Text): Null {
    if file_exist(origin) {
        unsafe $chmod +x "{path}"$
        return true
    }

    echo "The file {$path} doesn't exist!"
    return false
}

To

pub fun make_executable(path: Text): Bool {
    if file_exist(path) {
        unsafe $chmod +x "{path}"$
        return true
    }

    echo "The file {path} doesn't exist!"
    return false
}

@Mte90
Copy link
Member Author

Mte90 commented Jun 18, 2024

So I have to removed the non-failable commands from this PR.

About make_symbolic_link I think that is better to rename it as create_symbolic_link to keep the consistency.

@Mte90
Copy link
Member Author

Mte90 commented Jun 18, 2024

Probably is missing a function to extract compressed files, download files and to search inside file content.
I will work on those the next days.

@Ph0enixKM Ph0enixKM linked an issue Jun 19, 2024 that may be closed by this pull request
src/std/main.ab Outdated Show resolved Hide resolved
@Mte90
Copy link
Member Author

Mte90 commented Jun 19, 2024

I did some changes and added the download command but I left a note for that.

src/std/main.ab Outdated Show resolved Hide resolved
Copy link
Member

@b1ek b1ek left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

every stdlib command should be covered by a test. the tests are in src/tests/stdlib.rs

added is_root and is_command
@Mte90
Copy link
Member Author

Mte90 commented Jun 19, 2024

every stdlib command should be covered by a test. the tests are in src/tests/stdlib.rs

I never developed with Rust but I guess that is enough to generate the bash and check that is always the same right?
Maybe this can be simplified instead of editing a rust file maybe a pure text file is more simplier?


I added also is_root and is_command and improved the download function.

@Ph0enixKM
Copy link
Member

@Mte90 if you want to write a standard function to download file, then maybe write one that detects available solution to download it like curl, wget, python and then use the strategy to do it:

if {
    not has_failed("curl") {
        // ... run curl command
    }
    not has_failed("wget") {
        // ... run wget command
    }
    not has_failed("python") {
        // ... run python command (and detect python version)
    }

@Mte90
Copy link
Member Author

Mte90 commented Jun 19, 2024

not has_failed("wget") {

I think that is better a command that check if exist instead to run it to see what is the error.
In this way we can avoid issues more easily, that is why I added the is_command function.

@Ph0enixKM
Copy link
Member

I think that is better a command that check if exist instead to run it to see what is the error. In this way we can avoid issues more easily, that is way I added the is_command function.

Sure! I just wanted to share an example of potential implementation

@Mte90 Mte90 requested a review from b1ek June 20, 2024 08:40
@Mte90
Copy link
Member Author

Mte90 commented Jun 20, 2024

So I think that another round of review for the function so I can try to do the unit tests :-)

@b1ek
Copy link
Member

b1ek commented Jun 22, 2024

There are some conflicts with cargo.lock/toml

ftfy <3

Copy link
Member

@b1ek b1ek left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

lgtm

@Mte90
Copy link
Member Author

Mte90 commented Jun 26, 2024

So I added 3 new commands:

  • write that can write or append content
  • get_env_var that echo the variable and in case doesn't exist read the variable from the .env file
  • load_env_file export a .env file variables

I am fighting with the compilation as we need $(echo "${!var}") to read the variable exported otherwise doesn't work and I am trying to understand how to generate that line in this way.
So right now 2 tests fails.

@Mte90 Mte90 mentioned this pull request Jun 26, 2024
@Mte90
Copy link
Member Author

Mte90 commented Jul 1, 2024

So the write function I added is gone as there was already file_write.

@Mte90
Copy link
Member Author

Mte90 commented Jul 1, 2024

I don't understand the error reported for the get_env_var test https:/Ph0enixKM/Amber/actions/runs/9745193207/job/26892528212?pr=185

Someone that can help my with the Amber code review? @Ph0enixKM

@Ph0enixKM
Copy link
Member

Ph0enixKM commented Jul 1, 2024

ERROR: Failed expression must be followed by a block or statement

This means that the expression you used is failable and you should handle the error @Mte90

@Mte90
Copy link
Member Author

Mte90 commented Jul 1, 2024

ERROR: Failed expression must be followed by a block or statement

This means that the expression you used is failable and you should handle the error @Mte90

In the meantime I fixed it, also now the CI should have a green check :-)

@Mte90 Mte90 requested a review from b1ek July 1, 2024 14:52
@Mte90
Copy link
Member Author

Mte90 commented Jul 1, 2024

This pr is ready :-)

Comment on lines +26 to +33
fn http_server() {
use tiny_http::{Server, Response};

let server = Server::http("127.0.0.1:8081").expect("Can't bind to 127.0.0.1:8081");
for req in server.incoming_requests() {
req.respond(Response::from_string("ok")).expect("Can't respond");
break;
}
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Whoa! Didn't expect http_server in the compiler. Does the curl actually download a file?

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

it is used only for tests and that part was implemented by @b1ek.

Because initially I did to download a website but the issue was that what is if the user is not connected to internet or the page is not available?

Copy link
Member

@Ph0enixKM Ph0enixKM left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Looks good to me

@Ph0enixKM Ph0enixKM merged commit 2baeaa1 into amber-lang:master Jul 1, 2024
1 check passed
Mte90 added a commit to Mte90/Amber that referenced this pull request Jul 3, 2024
* feat(std): more command

* removed functions

* download

* Update main.ab

added is_root and is_command

* feat(download): improved fallback

* amber fix

* feat(test): is_command

* feat(test): create_symbolic_link

* feat(test): added 3 tests

* feat(test): added 2 tests

* fix(test): download

* feat: spin up a small web server for download() test

* fix: regenerate Cergo.lock

* feat(env): 3 new functions

* fix(amber): now should compile

* fix(amber): now should compile

* fix(amber): clean

* fix(test): removed write

* fix(test): maybe

* fix(test): wip

* fix(test): wip

* ci(cache): now will use CI cache, so it will be more fast between jobs

---------

Co-authored-by: b1ek <[email protected]>
@Mte90 Mte90 deleted the std branch July 17, 2024 08:46
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.

[Feature] More built-in commands
5 participants