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

fix(lib): simplify and improve sh! macro #195

Merged
merged 1 commit into from
Sep 4, 2023

Conversation

jackstar12
Copy link
Contributor

working on #194 I noticed a issue with the sh macro when trying to execute a script that uses a pipe. Looking into the implementation I saw that it tries to manually parse out the commands, which is pretty error prone. so, instead of trying to manually parse the script and potentially making errors, let the actual sh command do the work correctly

@netlify
Copy link

netlify bot commented Aug 26, 2023

Deploy Preview for coffee-docs canceled.

Name Link
🔨 Latest commit 785e8ac
🔍 Latest deploy log https://app.netlify.com/sites/coffee-docs/deploys/64f5cc2a92771c0008f0eb87

}
return Err(CoffeeError::new(2, &content));
let mut cmd = Command::new("sh");
cmd.args(&["-c", &script]);
Copy link
Contributor

Choose a reason for hiding this comment

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

Ah nice trick!

Copy link
Contributor

@vincenzopalazzo vincenzopalazzo left a comment

Choose a reason for hiding this comment

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

LGTM

I have just a comment on the expect call, we can return an error and avoid panicking? if this panic happens in the httpd can cause problem (maybe)

thanks! VERY NICE IMPROVEMENT

coffee_lib/src/macros.rs Outdated Show resolved Hide resolved
@vincenzopalazzo
Copy link
Contributor

vincenzopalazzo commented Sep 2, 2023

@jackstar12 Are you still working on this? I would really love to merge went he review is addressed

@jackstar12
Copy link
Contributor Author

@jackstar12 Are you still working on this? I would really love to merge went he review is addressed

yes, I was on vacation this week, I`ll get to it in the next few days

instead of trying to manually parse the script and potentially making errors,
let the actual `sh` command do the work correctly
Copy link
Contributor

@vincenzopalazzo vincenzopalazzo left a comment

Choose a reason for hiding this comment

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

Thanks for working on this and fixing my bad sh macros!

ACK 785e8ac

@vincenzopalazzo vincenzopalazzo merged commit 35f56c9 into coffee-tools:master Sep 4, 2023
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.

2 participants