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

Jump to bottom

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

Closed
dmitshur opened this issue Sep 19, 2015 · 6 comments
Closed

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

dmitshur opened this issue Sep 19, 2015 · 6 comments
Labels
FeatureRequest FrozenDueToAge Tools This label describes issues relating to any tools in the x/tools repository.
Milestone
Unreleased

Comments

@dmitshur
Copy link
Contributor

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 dmitshur mentioned this issue Sep 19, 2015
Merged
4 tasks
@dmitshur
Copy link
Contributor Author

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.

@rsc rsc added this to the Unreleased milestone Oct 23, 2015
@dmitshur dmitshur mentioned this issue Feb 16, 2018
Closed
@dmitshur
Copy link
Contributor Author

dmitshur commented Feb 16, 2018 •
edited

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
@gopherbot gopherbot mentioned this issue Feb 16, 2018
Closed
@bradfitz
Copy link
Contributor

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 This label describes issues relating to any tools in the x/tools repository. label Sep 12, 2019
@heschi
Copy link
Contributor

heschi commented Nov 7, 2019

Having never seen this issue, I did most of this incidentally when I refactored to create the Resolver interface. I'm not sure that it makes sense to allow users to implement it, though.

@dmitshur
Copy link
Contributor Author

dmitshur commented Nov 21, 2019 •
edited

As an update from my side, I made this feature request 4 years ago only because the GopherJS Playground needed to fork this package. I haven't had any other need for it since then, and the GopherJS Playground fork isn't costly to maintain.

My suggestion from #12696 (comment) still stands. Maybe we should close this issue, since there isn't much demand for it? /cc @heschik

to create the Resolver interface

That exists in golang.org/x/tools/internal/imports as far as I see, not in golang.org/x/tools/imports that this issue is about. I hope it stays that way, to avoid committing to a public API that you can't change.

@heschi
Copy link
Contributor

heschi commented Nov 21, 2019

Yes, I don't think we want to allow users to implement Resolver. Given the lack of other traffic, let's close this.

@heschi heschi closed this as completed Nov 21, 2019
@golang golang locked and limited conversation to collaborators Nov 20, 2020
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Assignees
No one assigned
Labels
FeatureRequest FrozenDueToAge Tools This label describes issues relating to any tools in the x/tools repository.
Projects
None yet
Milestone
Unreleased
Development

No branches or pull requests
5 participants
@bradfitz @rsc @dmitshur @gopherbot @heschi