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

Add - as placeholder for stdin/stdout #173

Merged
merged 11 commits into from
Jul 25, 2024

Conversation

b1ek
Copy link
Member

@b1ek b1ek commented Jun 11, 2024

this should be useful when amber is used like this:

let amber = Command::new("amber")
    .arg("-").arg("-")
    .pipe_to_stdin("import something from \"somewhere\"\necho something(\"hi!\")\n")
    .run();
let compiled = amber.get_stdout();
do_stuff_with_compiled_bash(compiled);

also this PR adds support for /dev/null as an output destination: if it is /dev/null, the compiled code will be discared (like a dry run or something)

@b1ek b1ek self-assigned this Jun 11, 2024
@arapower
Copy link
Contributor

$ amber -e 'echo "abc"'

How will the above command execution change after this change?

@Ph0enixKM
Copy link
Member

Ph0enixKM commented Jun 12, 2024

Let's add:

  • --silent - silence the output. It can't be --quiet because we have a syntax in amber to silence commands silent. This will make naming unified
  • --input - expects user to pass input to stdin instead of reading path. This is more readable than single - symbol.
amber --input [OUTPUT] # Outputs to a file
amber --input # Outputs to STDOUT
amber --input --silent # Never outputs

What do you think?

@b1ek
Copy link
Member Author

b1ek commented Jun 20, 2024

  • This is more readable than single - symbol.

the reason for using - is that many core unix programs treat it as stdin/stdout. like cat - will read from stdin

@Mte90
Copy link
Member

Mte90 commented Jul 9, 2024

I like that we offer both, - and --input as the first one is already a bash standard for this kind of stuff in various commands and the other one is more explicative.

@b1ek
Copy link
Member Author

b1ek commented Jul 9, 2024

maybe we should rather just leave it as - and reserve --input for future use? it seems like a nice keyword that can refer to almost anything

@Ph0enixKM
Copy link
Member

Ph0enixKM commented Jul 9, 2024

the reason for using - is that many core unix programs treat it as stdin/stdout. like cat - will read from stdin

I see. Okay we can use the - then. Let's still use the --silent for silencing output

@Mte90
Copy link
Member

Mte90 commented Jul 11, 2024

So to approve this PR we need just the conflicts fixed?

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.

@b1ek To merge this let's:

  • Modify the code to use the --silence instead
  • Resolve conflicts

src/main.rs Outdated Show resolved Hide resolved
@Ph0enixKM Ph0enixKM removed the request for review from boushley July 12, 2024 10:29
@Mte90
Copy link
Member

Mte90 commented Jul 16, 2024

So I tried to align the code to the latest changes, also added --silence so we can have both (also -) but I am not sure if works.

There aren't tests or how to use it.

@Ph0enixKM
Copy link
Member

@Mte90 sorry. It should be --silent. I made a mistake in my previous comment.

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.

@Mte90 I clarified what I meant

src/main.rs Outdated Show resolved Hide resolved
src/main.rs Outdated Show resolved Hide resolved
src/main.rs Outdated Show resolved Hide resolved
@Ph0enixKM
Copy link
Member

Ph0enixKM commented Jul 16, 2024

Use case scenarios for the CLI:

  • amber file.ab -> Runs the code and outputs
  • amber file.ab - -> Outputs the compiled bash code
  • amber file.ab file.sh -> Compiles the code to the destination
  • amber --silent file.ab -> Performs a dry run of compiler and the amber script
  • amber --silent file.ab - -> Performs a dry run of the compiler

Mte90 and others added 3 commits July 17, 2024 10:42
Co-authored-by: Phoenix Himself <[email protected]>
Co-authored-by: Phoenix Himself <[email protected]>
@b1ek
Copy link
Member Author

b1ek commented Jul 17, 2024

it will still break if the user uses /dev/null as the output file

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.

Typo

src/main.rs Outdated Show resolved Hide resolved
Co-authored-by: Phoenix Himself <[email protected]>
@Mte90 Mte90 requested a review from Ph0enixKM July 23, 2024 11:26
src/main.rs Outdated Show resolved Hide resolved
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.

Resolved conflicts

@Ph0enixKM Ph0enixKM requested review from Mte90, CymDeveloppement and KrosFire and removed request for arapower July 24, 2024 16:56
src/main.rs Outdated Show resolved Hide resolved
src/main.rs Outdated Show resolved Hide resolved
Mte90 and others added 2 commits July 25, 2024 11:23
@Mte90 Mte90 merged commit 1f7dc08 into amber-lang:master Jul 25, 2024
1 check passed
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.

5 participants