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

Add option to use cwd as package name instead of template #1124

Merged
merged 2 commits into from
Dec 17, 2016

Conversation

jimmyfrasche
Copy link
Contributor

This adds an option to use the directory name that contains a package as the name of the package instead of using a template, as requested in issue #1053

Copy link
Owner

@fatih fatih left a comment

Choose a reason for hiding this comment

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

Thanks for the PR. Looks good, made some comments though before we can merge it in.

let l:template_file = get(g:, 'go_template_file', "hello_world.go")
let l:template_path = go#util#Join(l:root_dir, "templates", l:template_file)
exe '0r ' . fnameescape(l:template_path)
$delete _
elseif l:package_name == -1 && l:go_template_no_file == 1
" cwd is now the dir of the package
let l:path = fnamemodify(getcwd(), ':t')
Copy link
Owner

Choose a reason for hiding this comment

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

We already have the package name, via l:package_name can you use that instead of using the directory? Because directory and package names can be different in Go.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I do not think I have communicated the intention of my change.

When there are existing files, l:package_name 1= -1, the current behavior is perfect.

When there are not, I do not want to use the template. However, I do want to use the directory name, which, by convention is the same name as the package.

The goal is for it to have it behave the same, whether there are files or not.

I'm sorry if I was unclear before.

Maybe the setting needs a different name to make that clearer?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

g:go_no_template_use_cwd or something like that?

Copy link
Owner

Choose a reason for hiding this comment

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

I understand your changes. Just we should use the name package instead of directory or cwd everywhere. A package might have a different name than a directory.

Do not change the logic as it's correct (just read your comment and it's clear now). But let us change the variable to go_template_use_package or go_template_use_pkg. Does it make sense for you? We don't use cwd or directory nowhere in vim-go because of the reason I've outlined above

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I see what you mean, but I think it would be confusing to a reader (of the docs or code) if the word directory isn't mentioned anywhere.

This change, essentially, guesses that you most likely want to use the name of the directory that a new package is created in as the name of that package.

Using package instead of directory in the docs means that the docs would have to say something like, "use the package name for the name for new packages".

That's confusing because package means two different things in the same sentence.

But something like, "use the directory name of the package as the name for new packages," makes it clear what the two different things are and how they relate.

This would be the first time the directory is mentioned in the docs, but it would also be the first time that it was relevant.

Copy link
Owner

Choose a reason for hiding this comment

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

Using package instead of directory in the docs means that the docs would have to say something like, "use the package name for the name for new packages".

I disagree, it's up to us how to write it down. I went over again and here are the fixed doc sentences:

When a new Go file is created, vim-go automatically fills the buffer content
with a Go code template. By default the template under
`templates/hello_world.go` is used. This can be changed with the
|'g:go_template_file'| setting.

If the new file is created in an already prepopulated package (with other Go
files), in this case a Go code template with only the Go package declaration
(which is automatically determined according to the current package) is added.

To use always the package name instead of the template (`tempaltes/hello_world.go`)
when new Go file is created, enable the |'g:go_template_use_pkg'| setting.

By default it is enabled.
>
  let g:go_template_autocreate = 1
<

and

Specifies that, rather than using a template, the package name is used 
if a new Go file is created. Checkout |'g:go_template_autocreate'| for more 
info. By default the `hello_world.go` file is used.

>
  let g:go_template_use_pkg = 0
<

As you see, it's not complicated and it makes sense to use it this way. It's understandable and there is nowhere the notion of directory. If you just update the docs with the ones above and change the variable name to go_template_use_pkg we're good to go.

let l:path = fnamemodify(getcwd(), ':t')
let l:content = printf("package %s", l:path)
call append(0, l:content)
$delete _
Copy link
Owner

Choose a reason for hiding this comment

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

Per my comment above, I think you don't need three separate clauses. The following is sufficient:

  if l:go_template_no_file != 1 || l:package_name == -1
    let l:template_file = get(g:, 'go_template_file', "hello_world.go")
    let l:template_path = go#util#Join(l:root_dir, "templates", l:template_file)
    exe '0r ' . fnameescape(l:template_path)
    $delete _
  else
    let l:content = printf("package %s", l:package_name)
    call append(0, l:content)
    $delete _
  endif

Copy link
Owner

Choose a reason for hiding this comment

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

Please neglect this comment.

*'g:go_template_no_file'*

Specifies that, rather than using a template, the directory name is used as the
package name if a new Go file is created. Checkout |'g:go_template_autocreate'| for more info. By default it is disabled.
Copy link
Owner

Choose a reason for hiding this comment

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

Should be .. the package name is used if a new Go file is created

Also can you wrap this just like the other paragraphs.

Copy link
Owner

Choose a reason for hiding this comment

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

This still applies.

@@ -1438,6 +1438,10 @@ with a Go code template. By default the template under
`templates/hello_world.go` is used. This can be changed with the
|'g:go_template_file'| setting.

To use the package's directory as the name of the package instead of a template
Copy link
Owner

Choose a reason for hiding this comment

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

Should be To use the package name instead of ...

Copy link
Owner

Choose a reason for hiding this comment

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

This still applies.

@fatih fatih merged commit 9f9b937 into fatih:master Dec 17, 2016
@fatih
Copy link
Owner

fatih commented Dec 17, 2016

Thanks @jimmyfrasche 👍

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.

2 participants