Skip to content
New issue

Have a question about this project? # for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “#”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? # to your account

Implement new flag for printing the executed commands #1068

Open
wants to merge 11 commits into
base: master
Choose a base branch
from

Conversation

ilmari-h
Copy link

@ilmari-h ilmari-h commented Jul 22, 2022

Implementation for #1042, flag --print-exec
The flag causes the commands that are going to be executed to first be printed with colored highlighting. Highlighting uses the same logic as when running fd and just listing results. The highlighting is also applied to templates.

TODO:

  • Change print to happen in sequence with commands, in the same locked buffer output before command is printed
  • Resolve how the option should be named
  • Support also printing when running in batch mode
  • Update docs
  • Optimize & clean

@matu3ba
Copy link

matu3ba commented Jul 26, 2022

Questions: 1. Why not using -v as verbose output ? 2. Is this use case very frequent to justify a 1 letter command abbreviation?

@ilmari-h
Copy link
Author

Questions: 1. Why not using -v as verbose output ? 2. Is this use case very frequent to justify a 1 letter command abbreviation?

  1. You mean, why not verbose instead of print-exec? I just personally liked the print-exec suggestion in the linked issue. I'd intuitively think verbose would refer to also verbose printing of other things aside from just executed commands.
  2. Yeah, I wasn't sure what the convention was in this repo. But looking at other options, you're right. I think a short option isn't required.

So the answer to both: kind of just went by what was proposed in the issue. At least for the short option, I think I'll remove that

@jessestricker
Copy link

I agree with you and have thus updated the proposal.

@ilmari-h ilmari-h marked this pull request as ready for review August 1, 2022 16:52
@ilmari-h
Copy link
Author

ilmari-h commented Aug 2, 2022

Fixed the last style warnings, now ready for review. Hope the implementation is satisfactory, tried to keep changes to existing logic minimal and did my best to make sure the runtime performance is not impacted with unnecessary overhead, at least when not using the new flag.

@ilmari-h
Copy link
Author

ilmari-h commented Aug 3, 2022

Pushed a couple more commits, noticed printing was broken for multiple command instances. Now works with those as well.

@sharkdp
Copy link
Owner

sharkdp commented Sep 11, 2022

Thank you very much for your contribution!

This looks good from a very high level, but I'll probably need some more time to review this in detail. If there are any things in here that could be split out into a separate PR (and are useful independent from this feature), that would be very much appreciated.

@danielkrajnik
Copy link

danielkrajnik commented May 24, 2023

wish I had something more constructive to say other than I'm just hyped for this feature and looking forward to it being merged 👀 . Sounds much better than keep --exec sh -c 'echo "{}"; ...'ing...

# for free to join this conversation on GitHub. Already have an account? # to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

5 participants