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

Immediate follow #595

Merged
merged 2 commits into from
Feb 4, 2025
Merged

Immediate follow #595

merged 2 commits into from
Feb 4, 2025

Conversation

Nukesor
Copy link
Owner

@Nukesor Nukesor commented Feb 1, 2025

Resolves #592

@Nukesor Nukesor added t: Feature A new feature that needs implementation s: Pueue-lib This issue touches the pueue-lib library s: Daemon This issue touches pueue daemon s: Client This issue touches the pueue client labels Feb 1, 2025
@Nukesor Nukesor self-assigned this Feb 1, 2025
Copy link

github-actions bot commented Feb 1, 2025

Test Results

  3 files   19 suites   3m 25s ⏱️
174 tests 174 ✅ 0 💤 0 ❌
366 runs  366 ✅ 0 💤 0 ❌

Results for commit ccd3cb2.

♻️ This comment has been updated with latest results.

Copy link

codecov bot commented Feb 1, 2025

Codecov Report

Attention: Patch coverage is 85.15625% with 19 lines in your changes missing coverage. Please review.

Project coverage is 80.89%. Comparing base (117f2bd) to head (ccd3cb2).
Report is 6 commits behind head on main.

Files with missing lines Patch % Lines
pueue/src/client/client.rs 84.11% 17 Missing ⚠️
pueue/src/client/cli.rs 0.00% 1 Missing ⚠️
pueue_lib/src/format.rs 83.33% 1 Missing ⚠️
Additional details and impacted files
@@            Coverage Diff             @@
##             main     #595      +/-   ##
==========================================
+ Coverage   80.80%   80.89%   +0.09%     
==========================================
  Files          74       75       +1     
  Lines        5912     5942      +30     
==========================================
+ Hits         4777     4807      +30     
  Misses       1135     1135              

☔ View full report in Codecov by Sentry.
📢 Have feedback on the report? Share it here.

@Nukesor Nukesor force-pushed the immediate-follow branch 2 times, most recently from 3553aac to 07a8dea Compare February 2, 2025 21:51
Copy link

@katrinafyi katrinafyi 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! Thanks for working on this :)

Tested:

  • follow works with fast command: ./pueue add -i --follow -- 'ls'
  • follow continues command in background on ctrl+c
  • follow requires immediate flag to be set

I have a few very small comments. You can think about these if you want or ignore them.

  • the error message when using --follow without -i might be unclear. it says "error: the following required arguments were not provided: --immediate", but it's not obvious that --immediate is only required when --follow is given. i don't know if this is easy to change in the argument parsing library
  • if you Ctrl+C while following, it might not be obvious that the task continues. for comparison, when pressing Ctrl+Z in a shell, it prints ^Zfish: Job 2, 'sleep 100' has stopped
  • i'm not sure if this was deliberate, but the new task ID is not shown. i think it's useful to know the ID for using with other commands. a message like New task added (id 18). (same as using add without follow) could also help address the previous point.

Thanks again!

@Nukesor
Copy link
Owner Author

Nukesor commented Feb 3, 2025

  1. That's a limit of the CLI library and I think the error is clear enough.
  2. Just to be clear. Pueue does not aim to re-implement or replace the background task logic of the shell. I know that nushell uses it for that and that you're using it as a fancy & but that's not what it has been designed for.
    To be honest, I'm already starting to get a bad feeling about this feature request, because this is exactly the type of user behavior that'll be attracted by this feature, leading to more feature requests of this type.
    I'll reconsider again whether I'll actually merge this.
  3. The current behavior exactly matches the --follow behavior, which is to print the exact stdout output to the shell. Prepending any other output would potentially break pipes into other commands. E.g. somebody pipes the command's json output into jq or some follow-up command.

@katrinafyi
Copy link

Re (3), I think the output should have the output of add then follow, since it does the tasks of both. I don't know if it makes sense to pipe --follow to another command, as the pipe would be broken if the command is Ctrl+C'ed. Printing the ID still lets you do a pipe by manually using pueue follow ID | ....

Re (2), I understand.

@Nukesor
Copy link
Owner Author

Nukesor commented Feb 4, 2025

  1. Hmm. I guess a good alternative to that would be to print all of Pueue's info/error log output to stderr, which would be a good practice anyway.

@Nukesor
Copy link
Owner Author

Nukesor commented Feb 4, 2025

I changed the behavior to print the add output to stderr and the actual log output to stdout so the piping usecase is still viable.

I'll also merge this MR for the time being, but I don't plan to add any further features that aid Pueue mimicking as a shell background task manager. Stuff like that is the job of the shell itself :)

@Nukesor Nukesor merged commit d2c116e into main Feb 4, 2025
19 checks passed
@Nukesor Nukesor deleted the immediate-follow branch February 4, 2025 14:55
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
s: Client This issue touches the pueue client s: Daemon This issue touches pueue daemon s: Pueue-lib This issue touches the pueue-lib library t: Feature A new feature that needs implementation
Projects
None yet
Development

Successfully merging this pull request may close these issues.

feature suggestion: --folow argument for pueue add
2 participants