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

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
Labels
FrozenDueToAge NeedsDecision Feedback is required from experts, contributors, and/or the community before a change can be made.

Comments

@moznion
Copy link

moznion commented Oct 7, 2017 •
edited

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 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 Feedback is required from experts, contributors, and/or the community before a change can be made. label Oct 7, 2017
@mvdan
Copy link
Member

mvdan commented Oct 7, 2017

/cc @griesemer

@moznion
Copy link
Author

moznion commented Oct 7, 2017 •
edited

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 commented Oct 8, 2017 •
edited

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

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 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 commented Oct 8, 2017 •
edited

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 commented Oct 9, 2017 •
edited

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

@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 commented Oct 11, 2017

@superstas Sure, I'll do that.

@gopherbot
Copy link

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 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 as completed Oct 30, 2017
@nodo nodo mentioned this issue Jan 26, 2018
Closed
at15 added a commit to reikalang/rcl that referenced this issue Mar 23, 2018
@at15
[go] Move generated parser to its own folder
1e6ec9c 
- if we put it at project root and run gofmt ., vendor folder will get
formatted as well
  - golang/go#22173
@mwhooker mwhooker mentioned this issue May 2, 2018
Merged
wking added a commit to wking/openshift-installer that referenced this issue Aug 24, 2018
@wking
hack/go-fmt: Make it easy to auto-format Go
9df0ec0 
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
@wking
hack/go-fmt: Make it easy to auto-format Go
459a388 
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 wking mentioned this issue Aug 24, 2018
Merged
wking added a commit to wking/openshift-installer that referenced this issue Aug 24, 2018
@wking
hack/go-fmt: Make it easy to auto-format Go
94a2a8c 
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
@wking
hack/go-fmt: Make it easy to auto-format Go
87b3e17 
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.
Assignees
No one assigned
Labels
FrozenDueToAge NeedsDecision Feedback is required from experts, contributors, and/or the community before a change can be made.
Projects
None yet
Milestone
No milestone
Development

No branches or pull requests
6 participants
@rsc @superstas @moznion @mvdan @robpike @gopherbot