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

:GoAlternate and related documentation/mappings #704

Merged
merged 1 commit into from
Feb 4, 2016
Merged

:GoAlternate and related documentation/mappings #704

merged 1 commit into from
Feb 4, 2016

Conversation

nhooyr
Copy link
Contributor

@nhooyr nhooyr commented Jan 28, 2016

In #684 a golang alternate plugin was presented in which it was easy to
alternate between the implementation and testing code. This commit adds
support for that in vim-go. It also adds related documentation and mappings.

@nhooyr
Copy link
Contributor Author

nhooyr commented Jan 29, 2016

I think its better to have a variable to control the default positioning of the alternate file like GoRun rather then how I pass arguments. I'm gonna change it.

@nhooyr
Copy link
Contributor Author

nhooyr commented Jan 30, 2016

Ok so I changed it and added a setting named g:go_alternate_mode. I think we should rename these settings (g:go_term_mode) to instead use a suffix of cmd as thats what they really are. They aren't a mode. Its just simply the command used to open the new file.

I've updated the documentation on g:go_term_mode to reflect that. Its much more clear. There was also a mistake I corrected in the README.md where you cannot set g:go_term_mode to tab as that isn't the command to open a new tab. Its much more clear to say its a command.

It also escapes the filename and other stuff.


" By default create the test file if it does not exist
if !exists("g:go_create_test_file")
let g:go_create_test_file = 1
Copy link
Owner

Choose a reason for hiding this comment

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

Can you disable this? It should give an error if the file doesn't exists. Having it enabled by default will cause to create unwanted files. We should be very simple in the features and not disturb people's workflows.

Copy link
Owner

Choose a reason for hiding this comment

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

Ok I think we don't need this setting at all. Let's do this way. If e bang is passed it should force open the test file, if not passed it should give an error. Example:

Tries to open foo_test.go, if it doesn't exists it echoes an error:

:GoAlternate

Tries to open foo_test.go, if it doesn't exists it creates the foo_test.go file.

:GoAlternate!

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Thing is, its not really creating a file, it just create a buffer with the name of the file. Didn't want to do that? Pretty simple to just delete the new buffer. I'm using the bang right now so that if the current buffer isn't saved you can switch with the bang, so it matches other commands and their behaviour. Plus its nice for mappings so you don't have to type the entire command just to create the new file.

Copy link
Owner

Choose a reason for hiding this comment

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

I think we should change that. The bang should create a file if passed if not it should give an error: no test file found. Call :GoAlternate! to create one. With that also remove the let g:go_create_test_file = 1 option completely.

@fatih
Copy link
Owner

fatih commented Feb 1, 2016

Hi @nhooyr Good job 👍 I have a some comments, which I think makes it much more simpler. Let me know once you finished and I'll take another stab.

@nhooyr
Copy link
Contributor Author

nhooyr commented Feb 1, 2016

I also updated a comment for the syntax highlighting to reflect some new stuff.

@nhooyr
Copy link
Contributor Author

nhooyr commented Feb 4, 2016

@fatih its been a few days, what do you think now.

@fatih
Copy link
Owner

fatih commented Feb 4, 2016

@nhooyr I also want to use it, but I've just arrived home (Turkey). I didnt' see my family for 6 weeks, and because of my full time job recently it's hard to look ;)

to open the file.

If [!] is given then it switches to the new file even if the current file
isn't saved.
Copy link
Owner

Choose a reason for hiding this comment

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

This also needs to be updated with the new behavior.

@nhooyr
Copy link
Contributor Author

nhooyr commented Feb 4, 2016

@fatih sorry I just wanted to voice my concerns before I implemented it as you wanted. It's done now.

function! go#alternate#Switch(bang, cmd)
let l:file = go#alternate#Filename(fnameescape(expand("%")))
if !filereadable(l:file) && !a:bang
echoerr "couldn't find " . file
Copy link
Owner

Choose a reason for hiding this comment

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

Just tested this and this should be change (we don't use it as it's not a good UX command). See:

image

This is better:

image

You can use the following to achieve it:

    redraws! | echon "vim-go: " | echohl ErrorMsg | echon "couldn't find ".file | echohl None

Copy link
Contributor Author

Choose a reason for hiding this comment

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

@fatih Fixed

In #684 a golang alternate plugin was presented in which it was easy to
alternate between the implementation and testing code. This commit adds
support for that in vim-go. It also adds related documentation and
mappings. I also fixed some documentation issues to make g:go_term_mode
more clear.
@nhooyr
Copy link
Contributor Author

nhooyr commented Feb 4, 2016

@fatih fixed the error message.

@fatih
Copy link
Owner

fatih commented Feb 4, 2016

LGTM. Thanks @nhooyr nice work 👍

fatih added a commit that referenced this pull request Feb 4, 2016
:GoAlternate and related documentation/mappings
@fatih fatih merged commit aecdb50 into fatih:master Feb 4, 2016
@nhooyr
Copy link
Contributor Author

nhooyr commented Feb 4, 2016

@fatih I already noticed a bug. Well i think it is a bug, filereadable() only checks if that file is readable on the disk so if you already swapped into the alternate test file and created it but didn't save, you have to use the bang still even though the buffer exists. Instead I'll add in some code that will switch to the buffer automatically.

@fatih
Copy link
Owner

fatih commented Feb 4, 2016

@nhooyr great catch. And no worries. This is why it's ok to merge to master. We can use it and fix other bugs until we release a new version.

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

2 participants