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

Closed
eikenb opened this issue Feb 11, 2018 · 7 comments
Closed

x/tools/imports: consider exporting fixImports #23782

eikenb opened this issue Feb 11, 2018 · 7 comments
Labels
FrozenDueToAge NeedsDecision Feedback is required from experts, contributors, and/or the community before a change can be made. Tools This label describes issues relating to any tools in the x/tools repository.
Milestone

Comments

@eikenb
Copy link

eikenb 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 bradfitz added the NeedsDecision Feedback is required from experts, contributors, and/or the community before a change can be made. label Feb 12, 2018
@bradfitz
Copy link
Contributor

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
Copy link
Author

eikenb 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
Copy link
Author

eikenb 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
Copy link
Author

eikenb commented Feb 13, 2018

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

@dmitshur
Copy link
Contributor

dmitshur 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
Copy link
Author

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

@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

We have exposed an internal interface to the imports package that might suit your needs, but I don't think we're going to make it public. Feel free to fork x/tools to expose it, at your risk of course.

@heschi heschi closed this as completed Nov 7, 2019
@golang golang locked and limited conversation to collaborators Nov 6, 2020
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
FrozenDueToAge NeedsDecision Feedback is required from experts, contributors, and/or the community before a change can be made. Tools This label describes issues relating to any tools in the x/tools repository.
Projects
None yet
Development

No branches or pull requests

5 participants