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 function to automatically add package header #358

Open
wants to merge 1 commit into
base: master
Choose a base branch
from

Conversation

phikal
Copy link

@phikal phikal commented Apr 28, 2020

This patch fixes the minor annoyance of having to write out the package name when creating a new file. It scans all other file names and adds a package ... header if all other files share the same package line.

Even with packages with over 100 go files, this function takes less than 0.01 seconds to execute, and even less when compiled.

The current limitations are the only the first 4k byte are used to find a package name, and that it fails if only one file has a different file name (for example in golang.org/x/net/ipv4 there is a file called gen.go that's not part of the package, and thus prevents go-mode-add-package-line from doing anything). These could easily be fixed, but I'm not sure what the correct behavior is from the Go-side of things.

As this feature is intrusive, it's disabled by default. Toggling go-mode-add-package-line automatically adds it to the go-mode-hook.

@sfllaw
Copy link
Contributor

sfllaw commented Sep 24, 2021

The current limitations are the only the first 4k byte are used to find a package name, and that it fails if only one file has a different file name (for example in golang.org/x/net/ipv4 there is a file called gen.go that's not part of the package, and thus prevents go-mode-add-package-line from doing anything). These could easily be fixed, but I'm not sure what the correct behavior is from the Go-side of things.

I wonder if the correct thing to do would be to pick the package name that occurs most frequently?

@sfllaw
Copy link
Contributor

sfllaw commented Sep 24, 2021

Also, it looks like this patch doesn’t seem to handle package foo_test for _test.go files, inside package foo?

@phikal
Copy link
Author

phikal commented Sep 24, 2021

I wonder if the correct thing to do would be to pick the package name that occurs most frequently?

That could also be done, but I assume that there could be difficulties when there are an approximately equal number of packages in a directory. It would probably be best to provide a local-safe user option to decide how to do this.

Also, it looks like this patch doesn’t seem to handle package foo_test for _test.go files, inside package foo?

The last update should fix that.

@sfllaw
Copy link
Contributor

sfllaw commented Sep 24, 2021

I wonder if the correct thing to do would be to pick the package name that occurs most frequently?

That could also be done, but I assume that there could be difficulties when there are an approximately equal number of packages in a directory. It would probably be best to provide a local-safe user option to decide how to do this.

There are four cases that all seem to have a sensible default:

  1. The buffer represents the first .go file in the directory.
    1. In this case, it seems sensible to guess that the package name is the directory name.
  2. There is only one package name that shows up in the directory.
    1. Use that name. This is what your code does now.
  3. There are multiple packages in the directory, but one of them is most frequent.
    1. Use the most frequent name.
  4. There are two package names that have the most .go files in the directory.
    1. If one of the package names matches the directory name, that is probably the best default.
    2. Otherwise, pick one of the most frequent names at random.

@muirdm
Copy link
Collaborator

muirdm commented Sep 24, 2021

FYI gopls provides completion for the package statement now in empty files.

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