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

cmd/gofmt: ignore vendor directory #22173

Closed
moznion opened this issue Oct 7, 2017 · 12 comments
Closed

cmd/gofmt: ignore vendor directory #22173

moznion opened this issue Oct 7, 2017 · 12 comments

Comments

@moznion
Copy link

@moznion moznion commented Oct 7, 2017

Go's dependency management mechanism treats vendor directory specially to import external packages.
Currently, the gofmt command formats files even if those are managed under the vendor.
I think gofmt command should ignore the files under the vendor.

How does it feel?

(I wrote PoC patch here: https://github.com/golang/go/compare/master...moznion:gofmt_ignore_vendor_dir?expand=1)

@mvdan
Copy link
Member

@mvdan mvdan commented Oct 7, 2017

Have you tried go fmt? gofmt works on files and directories, not packages, so I'm not sure if it should treat vendor directories in a special way.

@mvdan mvdan changed the title Ignore `vendor` directory on gofmt command cmd/gofmt: ignore vendor directory Oct 7, 2017
@mvdan mvdan added the NeedsDecision label Oct 7, 2017
@mvdan
Copy link
Member

@mvdan mvdan commented Oct 7, 2017

@moznion
Copy link
Author

@moznion moznion commented Oct 7, 2017

Yes, I tried to use (and also I'm using) go fmt command. It usually works no problems.
However, in my understand go fmt is a subset of gofmt command so go fmt is limited in function compared with "gofmt".

Actually, go fmt --help said as follows:

To run gofmt with specific options, run gofmt itself.

Therefore, I feel it is convenient if gofmt also ignores the vendor (or provide such an option).

@robpike
Copy link
Contributor

@robpike robpike commented Oct 8, 2017

It is not a subset, it is a wrapper. The important difference between go fmt and gofmt is the list of packages passed to the binary by go fmt, a list that for example excludes vendored packages.

@superstas
Copy link
Contributor

@superstas superstas commented Oct 8, 2017

In addition to the discussion above.
gofmt is able to simplify code, that's a terrific feature.

There is need for formating and simplification code at the same time ( excluding vendor ) .
It would be great to enable somehow the simplification feature in go fmt in that case.

@moznion
Copy link
Author

@moznion moznion commented Oct 8, 2017

It is not a subset, it is a wrapper. The important difference between go fmt and gofmt is the list of packages passed to the binary by go fmt, a list that for example excludes vendored packages.

Yes, it’s right. My expression was wrong, but I understand the behavior of both.

There is need for formating and simplification code at the same time ( excluding vendor ) .
It would be great to enable somehow the simplification feature in go fmt in that case.

That is a good point.
I think that it is good if the function of the go fmt is expanded with options or other ways. I don't have a reason to stick to gofmt.
If there are plans to make such changes, I have an intention to contribute to it.
(Additionally, if that is the case; I feel I want a dryrun option for the go fmt command)

@robpike
Copy link
Contributor

@robpike robpike commented Oct 8, 2017

If there is a resolution to this, it should be to let go fmt have access to all the flags of gofmt, not to change gofmt to track changes in the go command.

@moznion
Copy link
Author

@moznion moznion commented Oct 9, 2017

If there is a resolution to this, it should be to let go fmt have access to all the flags of gofmt, not to change gofmt to track changes in the go command.

I agree this.
I tried writing a PcC code based on this opinion: https://github.com/golang/go/compare/master...moznion:fmt_gofmt_flag?expand=1

@superstas
Copy link
Contributor

@superstas superstas commented Oct 11, 2017

@moznion could you please send your changes using regular flow, otherwise I'm afraid it wouldn't be reviewed
https://golang.org/doc/contribute.html

@moznion
Copy link
Author

@moznion moznion commented Oct 11, 2017

@superstas Sure, I'll do that.

@gopherbot
Copy link

@gopherbot gopherbot commented Oct 11, 2017

Change https://golang.org/cl/70030 mentions this issue: cmd/gofmt: Make all of gofmt's functions to be available from go fmt command

@rsc
Copy link
Contributor

@rsc rsc commented Oct 23, 2017

I really hope that vendor directories will diminish in importance once proper package management arrives. Let's just not do anything special here for gofmt of vendor directories.

@rsc rsc closed this Oct 30, 2017
at15 added a commit to reikalang/rcl that referenced this issue Mar 23, 2018
- if we put it at project root and run gofmt ., vendor folder will get
formatted as well
  - golang/go#22173
wking added a commit to wking/openshift-installer that referenced this issue Aug 24, 2018
So users and CI tooling can keep our Go clean :).

The find call avoids formatting .build/* (Bazel output) and vendor/*
(controlled upstream), because gofmt has no special vendor/ handling
built in [1].

[1]: golang/go#22173 (comment)
wking added a commit to wking/openshift-installer that referenced this issue Aug 24, 2018
So users and CI tooling can keep our Go clean :).

The find call avoids formatting .build/* (Bazel output) and vendor/*
(controlled upstream), because gofmt has no special vendor/ handling
built in [1].

Formatting our Go and then using 'git diff' to show the changes (and
error if there were any) makes it easy for:

* CI to see if there were issues (because of the exit code).
* Users to see the required changes in the CI logs (because of the
  output diff).

[1]: golang/go#22173 (comment)
wking added a commit to wking/openshift-installer that referenced this issue Aug 24, 2018
So users and CI tooling can keep our Go clean :).

The find call avoids formatting .build/* (Bazel output) and vendor/*
(controlled upstream), because gofmt has no special vendor/ handling
built in [1].

Formatting our Go and then using 'git diff' to show the changes (and
error if there were any) makes it easy for:

* CI to see if there were issues (because of the exit code).
* Users to see the required changes in the CI logs (because of the
  output diff).

[1]: golang/go#22173 (comment)
wking added a commit to wking/openshift-installer that referenced this issue Aug 24, 2018
So users and CI tooling can keep our Go clean :).

The find call avoids formatting .build/* (Bazel output) and vendor/*
(controlled upstream), because gofmt has no special vendor/ handling
built in [1].

Formatting our Go and then using 'git diff' to show the changes (and
error if there were any) makes it easy for:

* CI to see if there were issues (because of the exit code).
* Users to see the required changes in the CI logs (because of the
  output diff).

[1]: golang/go#22173 (comment)
@golang golang locked and limited conversation to collaborators Oct 30, 2018
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Projects
None yet
Linked pull requests

Successfully merging a pull request may close this issue.

None yet
6 participants
You can’t perform that action at this time.