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

Show multiline errors #1456

Merged
merged 3 commits into from
Sep 18, 2017
Merged

Show multiline errors #1456

merged 3 commits into from
Sep 18, 2017

Conversation

arp242
Copy link
Contributor

@arp242 arp242 commented Sep 15, 2017

Unlike :echo, :echom is rather stupid. It will display a newline as
^@ or <00> and tabs as ^I or <09> (depending on the display
setting).

This some messages display nicer, for example :GoBuild -- looked
rather confusing before, and looks as expected now.

Before:
2017-09-15-081920_1920x1080_scrot

After:
2017-09-15-081934_1920x1080_scrot

Needs some more testing (Vim 7.4, Neovim), but seems to work well.

This fixes some – but not all – of #1429.

@arp242 arp242 mentioned this pull request Sep 15, 2017
@fatih
Copy link
Owner

fatih commented Sep 15, 2017

lgtm

Copy link
Collaborator

@bhcleek bhcleek left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I agree that this makes things look better, but I am not sure I would expect a new message to be added to message history for each line in the message; I think we should consider carefully whether that's really what vim-go should be doing. Just food for thought...

I'd also like to recommend that EchoSuccess, EchoProgress, EchoInfo, and maybeEchoWarning not append to message history; I think those commands should be using echo instead of echom.

redraw | echohl ErrorMsg | echom "vim-go: " . a:msg | echohl None
endfunction
" Tabs display as ^I or <09>, so manually expand them.
let l:msg = map(l:msg, 'substitute(v:val, "\t", " ", "")')
Copy link
Collaborator

@bhcleek bhcleek Sep 15, 2017

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Perhaps tabs should be expanded according to the value of &ts?

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

We could, but a tab width of 8 is kind of standard in terminals, so I think that 8 spaces makes more sense here than the tabstop setting.

Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

SGTM

@arp242
Copy link
Contributor Author

arp242 commented Sep 15, 2017

I agree that this makes things look better, but I am not sure I would expect a new message to be added to message history for each line in the message; I think we should consider carefully whether that's really what vim-go should be doing.

Are there any downsides to this? As far as I know it's just how it appears on the screen and in :mess?

I'd also like to recommend that EchoSuccess, EchoProgress, EchoInfo, and maybeEchoWarning not append to message history; I think those commands should be using echo instead of echom.

Maybe? Not sure. I understand why someone would prefer that, but on the other hand I personally prefer to have a complete message history.

@bhcleek
Copy link
Collaborator

bhcleek commented Sep 15, 2017

The downside that I see is that message history is limited to 200 messages. Often, that may not be a problem, but for some messages it could very easily end up dropping necessary information (e.g. when :GoTest panics it may not be possible to see where the panic actually happened, depending on where the running goroutine was in message listing).

I don't feel strongly about either of these things, but wanted to throw it out there for consideration.

Unlike `:echo`, `:echom` is rather stupid. It will display a newline as
`^@` or `<00>` and tabs as `^I` or `<09>` (depending on the `display`
setting).

This some messages display nicer, for example `:GoBuild --` looked
rather confusing before, and looks as expected now.

This fixes some – but not all – of #1429.
Also combine declaration/assignment in one entry while I'm here.
@arp242 arp242 merged commit 0371da6 into fatih:master Sep 18, 2017
@arp242 arp242 deleted the multiline-err branch September 18, 2017 05:14
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.

3 participants