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

FEATURE: flag to suppress the command output #338

Merged
merged 2 commits into from
Jan 27, 2025

Conversation

propilideno
Copy link
Contributor

Description

Sometimes, we want to get the output of the pet exec into a shell variable. like:

FOO=$(pet exec)
echo $FOO

but actually it's not possible, because actually we do not have flags to supress the command output after a exec

fmt.Printf("> %s\n", command)

Proposed Solution

I would like to use -q as --quiet but as in this project we adopted --query with -q, looks better to use --silent and -s to enable this feature.

FOO=$(pet exec -s)
echo $FOO

Terms

By submitting this pull request, I confirm that you can use, modify, copy, and redistribute this contribution, under the terms of your choice.

@RamiAwar
Copy link
Collaborator

Sorry for taking so long to review this, was on vacation for a while.

I like this. Two tiny comments:
1- You think 'silent' is descriptive enough? I feel like it might not be very clear, could be maybe understood as suppressing logs or errors, not output. What do you think of '--no-output' or '--suppress-output'? I don't like them much but worth bringing up.

2- Can you add tests for this? Something that checks that the command is still running but no output is getting written when this flag is provided.

@RamiAwar RamiAwar self-requested a review January 21, 2025 07:44
@propilideno
Copy link
Contributor Author

No problems, don't worry with it.

  1. Yes, i think silent is descriptive enough because curl, that is a widely adopted tool uses the same notation. And looks semantically nice because: "execute it silently".

You can verify it with curl --help or man curl.

  1. I agree with you that is important to build unit tests with it, because my current solution is suppressing the unique printf that is on the code, but when adding new features any contributor can perform a printf without checking the condition. Unfortunately, i have no time for it this month.

Best Regards,
Lucas Almeida.

@RamiAwar
Copy link
Collaborator

Alright, I'll try to help you out with this then 🙂 Hopefully I can find some time soon!

@RamiAwar
Copy link
Collaborator

Took a look at this today, seems more complicated than I expected.

The fzf command isn't setup properly for tests - it always launches in interactive mode.
Need to force it to use --filter when testing, and also getting only the first result. Quite some functionality will have to be updated.

So I'm going to merge this as is 😄

@RamiAwar RamiAwar merged commit 800dff4 into knqyf263:main Jan 27, 2025
4 checks 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.

2 participants