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: consider exporting fixImports #23782

Open
eikenb opened this issue Feb 11, 2018 · 6 comments

Comments

@eikenb
Copy link

commented Feb 11, 2018

This is a 3 character change that would make this module much more useful for people working on custom formatting solutions. Without this you need to format the code and then re-parse it to get the ast with the fixed imports which is very inefficient.

What version of Go are you using (go version)?

1.9/tip

Does this issue reproduce with the latest release?

Yes.

What operating system and processor architecture are you using (go env)?

N/A

What did you do?

Wanted to fix the imports at the ast.File level using the x/tools/imports lib.

What did you expect to see?

fixImports should be FixImports so it is public and can be used when working with ast's vs. files.

What did you see instead?

It is not public and I am forced to fork the project to use it.

@gopherbot gopherbot added this to the Unreleased milestone Feb 11, 2018

@bradfitz bradfitz changed the title x/tools/import - fixImports should be public (FixImports) x/tools/imports: consider exporting fixImports Feb 12, 2018

@bradfitz

This comment has been minimized.

Copy link
Member

commented Feb 12, 2018

This is a 3 character change

FWIW, the cost of this change isn't measured in characters or lines modified. It's a question of how much ongoing maintenance cost there is keeping this a stable interface for people to use long-term and whether that's even the API we want people using.

@eikenb

This comment has been minimized.

Copy link
Author

commented Feb 12, 2018

I understand the trade-offs and for my case think it is worth it (obviously). It seems like it follows in the spirit of the module, just at a lower level and the API for that function is very direct to work with when dealing with ASTs. Thank you for considering it.

@eikenb

This comment has been minimized.

Copy link
Author

commented Feb 12, 2018

If deemed a good change I'll be happy to submit a pull request with the change to help things along. I plan on maintaining this as a fork in the meantime, so I'll have it ready. Thanks again.

@eikenb

This comment has been minimized.

Copy link
Author

commented Feb 13, 2018

Went ahead and created the Pull Request to facilitate the decision.

eikenb added a commit to eikenb/tools that referenced this issue Feb 14, 2018
@dmitshur

This comment has been minimized.

Copy link
Member

commented Feb 16, 2018

This issue seems related to #12696. Although I don't know how exporting only fixImports helps.

The PR doesn't seem to help facilitate the decision, because it doesn't communicate why fixImports being exported makes sense and how it helps. It just implements it, but this issue is still in NeedsDecision state.

It would help if you provided more information about how it'd be used, etc.

@eikenb

This comment has been minimized.

Copy link
Author

commented Feb 17, 2018

The use case is when you want to update the imports in the AST without the text. For example I'm writing a custom formatter, to format code in a presentation like style, and wanted the import fix. I considered forking or just copying out the needed code, but thought it might be generally useful and it'd be better to not duplicate the functionality if unnecessary.

eikenb added a commit to eikenb/tools that referenced this issue Feb 19, 2018
imports: change fixImports into public FixImports
Includes a test to test FixImports directly and function documentation.

Fixes golang/go#23782

@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.