oilshell / oil

Oils is our upgrade path from bash to a better language and runtime. It's also for Python and JavaScript users who avoid shell!
http://www.oilshell.org/
Other
2.78k stars 150 forks source link

Error message shows wrong/confusing position for long commands #1973

Open greggyb opened 1 month ago

greggyb commented 1 month ago

When the command spills past one line, the error indicator does not point directly to the offending character of the command in question. You can see this in the image below, where the error is directly pointing to the middle of a literal string, rather than to the part of the pipeline and variable assignment that triggers this error.

Additionally, as you can see in the very long variable name, when the error occurs off of the first printed terminal line, then the error indicator also spills over the terminal width and you get empty space between the command and the indicator.

Columns do line up correctly, but the way this is indicated is not terribly readable. I expect this will mostly be an issue in anything minified or machine generated. I hope most humans aren't writing stuff like this on their own.

Running release 0.21.0 on Ubuntu 22.04.

image

Command is below. And 2 simpler reproductions (that also have no risk of mutating your system state (:, so perhaps more useful) is also below.

Original prompt from https://github.com/FrameworkComputer/linux-docs/blob/main/22.04-OEM-D.md

latest_oem_kernel=$(ls /boot/vmlinuz-* | grep '6.5.0-10..-oem' | sort -V | tail -n1 | awk -F'/' '{print $NF}' | sed 's/vmlinuz-//') && sudo sed -i.bak '/^GRUB_DEFAULT=/c\GRUB_DEFAULT="Advanced options for Ubuntu>Ubuntu, with Linux '"$latest_oem_kernel"'"' /etc/default/grub && sudo update-grub && sudo apt install zenity && mkdir -p ~/.config/autostart && [ ! -f ~/.config/autostart/kernel_check.desktop ] && echo -e "[Desktop Entry]\nType=Application\nExec=bash -c \"latest_oem_kernel=\$(ls /boot/vmlinuz-* | grep '6.5.0-10..-oem' | sort -V | tail -n1 | awk -F'/' '{print \\\$NF}' | sed 's/vmlinuz-//') && current_grub_kernel=\$(grep '^GRUB_DEFAULT=' /etc/default/grub | sed -e 's/GRUB_DEFAULT=\\\"Advanced options for Ubuntu>Ubuntu, with Linux //g' -e 's/\\\"//g') && [ \\\"\\\${latest_oem_kernel}\\\" != \\\"\\\${current_grub_kernel}\\\" ] && zenity --text-info --html --width=300 --height=200 --title=\\\"Kernel Update Notification\\\" --filename=<(echo -e \\\"A newer OEM D kernel is available than what is set in GRUB. <a href='https://github.com/FrameworkComputer/linux-docs/blob/main/22.04-OEM-D.md'>Click here</a> to learn more.\\\")\"\nHidden=false\nNoDisplay=false\nX-GNOME-Autostart-enabled=true\nName[en_US]=Kernel check\nName=Kernel check\nComment[en_US]=\nComment=" > ~/.config/autostart/kernel_check.desktop

And a safer one that just prints to the terminal:

myvar=$(printf "what a very long string that we have here, which forces the command line to wrap around the terminal width. long long long long long long long long long long long long long long long long long long long long long long long long long long long long long long long") && echo $myvar

And one that pushes the error indicator another line away (with error message shown in image below):

myverylonglonglonglonglonglonglonglonglonglonglonglonglonglonglonglonglonglonglonglonglonglonglonglonglonglonglonglonglonglonglonglonglonglonglonglonglonglonglonglonglonglonglonglonglonglonglonglonglonglonglonglonglonglonglonglonglonglonglonglonglonglonglonglonglonglonglonglonglonglonglonglonglonglonglonglonglonglonglonglonglonglonglonglonglonglonglonglonglongVarName=$(printf "abc") && echo $myverylonglonglonglonglonglonglonglonglonglonglonglonglonglonglonglonglonglonglonglonglonglonglonglonglonglonglonglonglonglonglonglonglonglonglonglonglonglonglonglonglonglonglonglonglonglonglonglonglonglonglonglonglonglonglonglonglonglonglonglonglonglonglonglonglonglonglonglonglonglonglonglonglonglonglonglonglonglonglonglonglonglonglonglonglonglonglonglonglongVarName

image

andychu commented 1 month ago

OK interesting

Yeah I suppose it is reasonable that if the error occurs on column 30, and the line is 900 characters long, then we cut off the line to 60 or 80 or 100?

That is pretty easily done and can mitigate cases like this

andychu commented 1 month ago

If the error is on column 700 out of 900, then it might still look bad

But I think the rough heuristic may still be worth it, it can improve many cases


Although in theory I suppose you could show columns 650-750, but maybe let's start with the simpler thing

greggyb commented 1 month ago

I think truncating makes sense as a first step. Some additional thoughts as I'm thinking about this:

  1. Coloring or using an underlined or bold font for the error.
    • Cons
      • dependency on terminal support
      • bad for logged output
    • Pros
      • supports any line length
      • In an interactive terminal, will stay with any reflowed output as terminal resizes
      • Would work well for headless shell (HTML styling could use the same color or font information)
  2. Hard-wrap to a reasonable size and insert the error marker directly under the relevant portion (example below)
    • Cons:
      • modifies user input
      • could still wrap poorly in a narrow output
    • Pros
      • should be equally friendly for interactive and logged output to a file
      • no terminal support required
theverylonglinevar=$(aaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaa)
                  ^ here's your problem
&& printf "and the rest of the command line ...." |
...