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

Report Success/Failure in the quickfix title #313

Open
wants to merge 1 commit into
base: master
Choose a base branch
from

Conversation

mvanderkamp
Copy link

Per this comment, I've added the status label part of the message to the quickfix title. This label is only added when the command completes, and when updating the quickfix list only the initial part of the title (before the status label) is used to check against the current quickfix list, so updates to the current quickfix list should still work as expected.

The status label is currently only added under the same circumstances that the normal "Success"/"Failure" message is shown.

@tpope
Copy link
Owner

tpope commented Jul 26, 2021

Been using this for a while with good results. I would have merged it some time ago, but (adapter/123) (Success:0) is just too much of a mess, and I haven't managed to think through what a replacement might look like. Eliminating the Success/0 redundancy would be a good start.

@mvanderkamp
Copy link
Author

Just to be clear, are you thinking:

  1. Only ever show the word "Success" or "Failure"
  2. Only ever show the exit code
  3. On success show only either the word "Success" or the status, otherwise show both word and code since the exit code can be useful in case of failure

@tpope
Copy link
Owner

tpope commented Jul 27, 2021

Undecided. My initial instinct was something like (adapter/12345=>0) but that ASCII arrow is pretty ugly (and I want to stick to ASCII to maximize portability, so something like or is off the table).

@tpope
Copy link
Owner

tpope commented Jul 27, 2021

I am okay to rethink the adapter/12345 part too, just so long as we retain something parseable and distinct.

@mvanderkamp
Copy link
Author

mvanderkamp commented Jul 28, 2021

What's the use case for showing handler and pid information in the title after the process has completed? Programmatically it is used to continue making sure that we uniquely identify lists in the quickfix history so that we can properly update them, but what does a user do with that information at that point?

If it's just for uniquely identifying quickfix lists, we could use the id of the list or set the context to achieve the same thing, and just show the exit status in the title instead.

@tpope
Copy link
Owner

tpope commented Jul 29, 2021

PID and adapter are in the title for continuity with the start message, and they're in the start message because they're useful for understanding what's going on. Continuity could be fulfilled by the PID alone though, so that opens things up a bit. I wouldn't even say continuity is a hard requirement, but I do like that if you run the same command several times in a row, each title is unique, helping to reduce confusion.

We're currently in the process of phasing out Vim 7 support, but even when we get to an 8.0 minimum version, that's not enough to guarantee support for the quickfix ID or context.

@mvanderkamp
Copy link
Author

Okay, that all makes sense, thank you!

What do you think of a format like: (12345/Failure[127])

  • Use the title up to (12345/ to find quickfix lists
  • While the command is running, put a _ placeholder in for the result label: (12345/_)
  • Only show the [127] exit code part if the command failed.

Also: should this be how the postfix is always formatted, or just for the quickfix title?

@tpope
Copy link
Owner

tpope commented Aug 12, 2021

Ngl I hate that ([]) nesting more than =>. Do we really need the exit code?

One thing I will say is that if we stick with Success/Failure, the natural placeholder is Running.

@mvanderkamp
Copy link
Author

I'm not attached to the exit code, I just thought it would be useful- I don't mind ditching it.

the natural placeholder is Running.

Haha good call, of course it is!

For example, instead of (tmux/12345), show (12345/Success), or
(12345/Running) if the command hasn't finished yet.

This is helpful in cases where it is difficult at a glance for a user to
tell whether their command succeeded just from reading the output.

This is updated in all cases where a postfix is added to command in
messages to the user, including the quickfix title.

For updating quickfix lists, the title up to the '/' character in the
postfix is used.
@mvanderkamp
Copy link
Author

Sorry I didn't notice there was a merge conflict! I think I've resolved it correctly (removed the added space before ! in the call to echo_truncated on line 1257 of autoload/dispatch.vim since the label is not shown in that part of the message anymore).

@tpope
Copy link
Owner

tpope commented Jan 18, 2022

Still not ready to act on this, but I can assure you I consider this mandatory for the next stable release, as it's impossible to tell if the job adapter is finished without it, assuming you miss the status message. A merge conflict won't block this.

@Kamilcuk
Copy link
Contributor

I'll just drop by for a +1

tpope added a commit that referenced this pull request Feb 11, 2024
@tpope
Copy link
Owner

tpope commented Feb 11, 2024

I still use the tmux adapter the majority of the time, but rest assured whenever I use the job adapter this continues to bother me!

We're currently in the process of phasing out Vim 7 support, but even when we get to an 8.0 minimum version, that's not enough to guarantee support for the quickfix ID or context.

One advantage of the long delay is enough time has passed that I think I'm willing to cave on this. I'm pushing a change to require the quickfix id for asynchronous dispatch to see if anyone complains.

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.

None yet

3 participants