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

feat(#118): improve error message for --execute #119

Open
wants to merge 9 commits into
base: main
Choose a base branch
from

Conversation

AlejandroSuero
Copy link

With these changes, the error messages will show error that occurred. Closes #118.

Screenshot 2024-09-04 at 21 23 47

Note

As you can see, it only shows the first line of the message.


Possible improvements:

  • Show the full message, current state:

Screenshot 2024-09-04 at 21 30 51

$ git defer
git: 'defer' is not a git command. See 'git --help'.

The most similar commands are
        rerere
        revert

@bashbunni
Copy link
Member

Hey @AlejandroSuero thank you so much for this contribution! Do you know why the full error message isn't displayed at the moment? This example output looks a bit rough
image

That said, I didn't reproduce the garbled output, so looks good??
image

Let me know if I'm missing anything :)

@AlejandroSuero
Copy link
Author

@bashbunni The code I committed has the version where it prints the first line of the error, because I ran into the problem with the garbled output.

I tested printing out to the terminal with fmt.Println not using errorDetails styles and it prints fine:

Screenshot 2024-09-05 at 21 02 12

Maybe the error is in the way lipgloss is rendering it, I will look into it and comeback.

@AlejandroSuero
Copy link
Author

@bashbunni as I thought, the problem is the way lipgloss is rendering it.

I'm kinda new to lipgloss so I don't know the specifics, but it seems that is calling Render on the whole string which for some reason causes problems.

I arrived to the solution of splitting the string and rendering it line by line like:

changes on the error rendering

Which results in the following output:

Screenshot 2024-09-05 at 21 32 05

Do you want me to commit it or is there a better way to achieve this with lipgloss?

main.go Outdated Show resolved Hide resolved
main.go Outdated Show resolved Hide resolved
error.go Outdated Show resolved Hide resolved
AlejandroSuero and others added 2 commits September 8, 2024 15:11
Also prints new line after error status.
Co-authored-by: ccoVeille <[email protected]>
@bashbunni
Copy link
Member

Sorry for the delay here, but I want to spend more time with that underlying issue in Lip Gloss before merging this. Ideally we can include the entire error message here with Lip Gloss styling 🙏

@bashbunni bashbunni self-assigned this Sep 16, 2024
@bashbunni
Copy link
Member

Hey update on this. I'm working to debug this in Lip Gloss and while I can reproduce the issue, for some reason the output is wrong, but the string even provided to fmt.Print() is correctly formatted and has all the content (or so it seems). I copied that string value to a file (included in the linked commit) then used less -R printf-val.txt which gave the expected output (not sure if the VS Code debugger includes newlines though. There are no newlines in there from what I saw.

4d15621

This is the result of less -R printf-val.txt
image

This is what prints to stdout
image

I'm working on this branch to debug this issue: https://github.com/charmbracelet/freeze/tree/bunni-improve-error-msg

I have a branch where the output formats correctly, but the solution is too hacky for my liking. I'd rather figure out why this is happening in Lip Gloss. fdcd0de

To debug in my debugger I'm running dlv debug --headless --api-version=2 --listen=127.0.0.1:43000 . -- -x "git defer" -c full and using the VS Code debugger with the following launch.json

{
    // Use IntelliSense to learn about possible attributes.
    // Hover to view descriptions of existing attributes.
    // For more information, visit: https://go.microsoft.com/fwlink/?linkid=830387

    // dlv debug --headless --api-version=2 --listen=127.0.0.1:43000 . -- -x "git defer" -c full
    "version": "0.2.0",
    "configurations": [
        {
            "name": "Connect to server",
            "type": "go",
            "request": "attach",
            "mode": "remote",
            "remotePath": "${workspaceFolder}",
            "port": 43000,
            "host": "127.0.0.1"
        }

    ]
}

@bashbunni
Copy link
Member

the issue is fixed when I comment out line 65 (l += s in the default case) of lipgloss master align.go. Once that is fixed on lipgloss then we can merge this :)

@bashbunni
Copy link
Member

Found the issue in Lip Gloss, have an open PR. Will take another look at this once that's merged charmbracelet/lipgloss#386

@AlejandroSuero
Copy link
Author

Thanks @bashbunni as I said on the Discord, I was a bit busy with take homes and got the job and started the next day after telling me that I got it 😄 , and now with all the onboarding and catching up, couldn't debug it myself. Good job finding the error 👍

@bashbunni
Copy link
Member

@AlejandroSuero no problem at all, you've already helped us lots with this PR + highlighting this issue in the first place. I hope the new gig is treating you well!

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.

improve error message for -x
3 participants