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

x/tools/imports: ability to customize behavior without needing to fork #12696

Open
dmitshur opened this issue Sep 19, 2015 · 3 comments

Comments

@dmitshur
Copy link
Member

commented Sep 19, 2015

Currently, the golang.org/x/tools/imports features some internal configuration options to make it possible to adjust it to run (in a limited capacity) in environments where there is no raw filesystem access (for example, App Engine):

// importPathToName returns the package name for the given import path.
var importPathToName = importPathToNameGoPath

// loadExports returns a list exports for a package.
var loadExports = loadExportsGoPath

// findImport searches for a package with the given symbols.
// If no package is found, findImport returns "".
// Declared as a variable rather than a function so goimports can be easily
// extended by adding a file with an init function.
var findImport = findImportGoPath

There's a few code generation files like mkindex.go to help pre-compute static indices of packages for such environments.

However, since those variables are unexported, the only current way to make changes there is to fork the entire golang.org/x/tools/imports package.

It's slightly more convenient in that a new file can be added with init() that overrides the internal variables, but the rest of the package will still be a fork that needs to be maintained and kept up to date.

Feature Request

What do you think about adding a means to override those internal configuration funcs (or provide your own implementations) and pre-computed indices. Perhaps via:

// Or "Context" or "Configuration" or whatever name works best.
type Config struct {
    ImportPathToName func(importPath string) (packageName string)
    LoadExports      func(dir string) map[string]bool
    FindImport       func(pkgName string, symbols map[string]bool) (string, bool, error)
    Stdlib           map[string]string
}

func (c *Config) Process(filename string, src []byte, opt *Options) ([]byte, error) { ... }

// Process formats and adjusts imports for the provided file.
// If opt is nil the defaults are used.
func Process(filename string, src []byte, opt *Options) ([]byte, error) {
    return defaultConfig.Process(filename, src, opt)
}

Or something like that, to make it possible to import golang.org/x/tools/imports and configure it to one's needs, without needing to fork it.

Additionally, it'd be great if the findImport, etc. implementations that currently rely on raw filesystem access and import low level packages like os could be moved into a separate file with a build tag like // +build !nacl to prevent them from being included on systems where those low level packages are not available.

Some context on where I'd like to use this can seen in gopherjs/gopherjs.github.io@40f830c, where I had to fork imports and customize for the GopherJS Playground. Since the code runs in a browser, a static index is used, and unused code importing os and sync is removed.

@dmitshur

This comment has been minimized.

Copy link
Member Author

commented Sep 19, 2015

By the way, I think this approach would be bad:

// Or "Context" or "Configuration" or whatever name works best.
type Config struct {
    ImportPathToName func(importPath string) (packageName string)
    LoadExports      func(dir string) map[string]bool
    FindImport       func(pkgName string, symbols map[string]bool) (string, bool, error)
    Stdlib           map[string]string
}

Because it makes internal implementation details a part of the API, making it harder to change.

Perhaps instead of exposing all internal details, imports could use build tags to have a fallback mode where it uses the pre-computed index of standard library for environments without raw filesystem access, and just a way of setting/adding to that pre-computed index.

@dmitshur

This comment has been minimized.

Copy link
Member Author

commented Feb 16, 2018

I suspect the best way to resolve this issue might be to decide that "forking imports is the best way to customize its behavior", because it seems very hard to come up with a better way.

@dmitshur dmitshur changed the title x/tools/imports: Feature Request: Ability to customize behavior without needing to fork. x/tools/imports: ability to customize behavior without needing to fork Feb 16, 2018

@bradfitz

This comment has been minimized.

Copy link
Member

commented Feb 26, 2018

I'm fine with some Config type, especially if it simplifies our internal Google version a bit.

I'd also want to declare the API as unstable in the package docs so we're free to evolve it as needed for a bit.

@gopherbot gopherbot added the Tools label Sep 12, 2019

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
4 participants
You can’t perform that action at this time.