-
-
Notifications
You must be signed in to change notification settings - Fork 1.2k
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
Delete multiple queues with a single cmd #727
base: master
Are you sure you want to change the base?
Conversation
f743f1f
to
00505ca
Compare
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Thanks for the PR, I think it's a good addition! There are a couple of changes I'd love you to make before we can merge it.
} | ||
return ink > inl | ||
}) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I think we should do a pre-flight sanity check, and bail out (printing correct error message) if index isn't found or parameter is bad. Not bailing out and not ignoring all entries will leave the user to an "unknown" state, which may cause them to retry the exact same command, minus the erraneous entry.
Example:
> queueshow
Queued jobs:
[0] : https://ffuf.io.fi/FUZZ (active job)
[1] : https://ffuf.io.fi/test1/FUZZ
[2] : https://ffuf.io.fi/another/FUZZ
[3] : https://ffuf.io.fi/job/FUZZ
> queuedel 1 word
[WARN] Not a number: word
> queuedel 1 2
User would expect jobs [1] and [2] to be deleted, but in reality the first command would have deleted job [1], moving the rest up an index, and the end result would be deletion of all three subsequent jobs: [1], [2], [3].
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
That's the current behavior, all non-number input has that warning
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
For instance:
> qd 111 a
[WARN] No such queued job. Use "queueshow" to list the jobs in queue
[WARN] Not a number: a
I still have to improve the "no such queued job"
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
It currently just shows the warning, but will delete other entries in the command. In case your example above would have had a valid queue entry, that valid entry would have been deleted.
While there would be a "Job XX successfully deleted!" - output, the thing that the user typically concentrates is the warning message.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Doing a more realistic test :
Queued jobs:
[0] : https://[REDACTED]/FUZZ (active job)
[1] : https://[REDACTED]/2/FUZZ
[2] : https://[REDACTED]/2016/FUZZ
[3] : https://[REDACTED]/2017/FUZZ
[4] : https://[REDACTED]/2019/FUZZ
[5] : https://[REDACTED]/2018/FUZZ
> qd 2 a 3
[INFO] Job successfully deleted!
[INFO] Job successfully deleted!
[WARN] Not a number: a
> q
Queued jobs:
[0] : https://[REDACTED]/FUZZ (active job)
[1] : https://[REDACTED]/2/FUZZ
[2] : https://[REDACTED]/2019/FUZZ
[3] : https://[REDACTED]/2018/FUZZ
> qd 2 a b 3 c
[INFO] Job successfully deleted!
[INFO] Job successfully deleted!
[WARN] Not a number: a
[WARN] Not a number: b
[WARN] Not a number: c
> q
Queued jobs:
[0] : https://[REDACTED]/FUZZ (active job)
[1] : https://[REDACTED]/2/FUZZ
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Non-number inputs are always at the end of the slice
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Note that "Job successfully deleted" is now "Job x successfully deleted"
pkg/interactive/termhandler.go
Outdated
i.Job.Output.Warning("Cannot delete the currently running job. Use \"queueskip\" to advance to the next one") | ||
} else { | ||
i.Job.DeleteQueueItem(index) | ||
i.Job.Output.Info("Job successfully deleted!") |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
It might be "jobs" in some cases after merging this PR ;)
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This is shown for all jobs :)
i.Job.DeleteQueueItem(index) | ||
i.Job.Output.Info("Job successfully deleted!") | ||
func (i *interactive) deleteQueue(in []string) { | ||
sort.Slice(in, func(k, l int) bool { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Add a comment to tell that a reverse sort based on Atoi values is what's accomplished here to keep the code readable
Oh, and we need to change the interactive mode help text (and a |
00505ca
to
7bc61b6
Compare
I've force-pushed the changes |
Description
We often have many queue to delete. This allows to run
queuedel 1 2 3 4
.Additonally
CONTRIBUTORS.md
.The file should be alphabetically ordered.
CHANGELOG.md