-
Notifications
You must be signed in to change notification settings - Fork 650
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
Import Gazel, BUILD file generator #51
Conversation
Added "fix" and "diff" modes
http://ci.bazel.io/job/PR-rules_go/41/BAZEL_VERSION=latest,PLATFORM_NAME=darwin-x86_64/console Looks to be a disk quota issue in Jenkins? |
err = merr | ||
} | ||
}() | ||
err = func() (err error) { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
In my experience, worrying about the error from the defer is overkill, and creates complexity without much gain.
I would have written (instead of line 31-49):
defer os.Remove(f.Name())
defer f.Close()
if _, err := f.Write(bzl.Format(file)); err != nil {
return err
}
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
done in #60
This is a long review so it will take some time. FYI all TODO should have // TODO(yugui): Respect exisiting manual configurations as well as possible On Tue, Jul 19, 2016 at 11:44 PM, Yuki Yugui Sonoda <
|
"example.com/another/sub", | ||
"example.com/repo_suffix", | ||
} { | ||
l, err := r.resolve(importpath, "") |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
combine with if stmt
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
done in #58
Thanks for creating this tool! +cc @damienmg @kchodorow @laurentlb I have one particular request: can we name this tool something other than "gazel?" I think the name is a bit confusing and have outlined a few reasons here. IIRC, we had some discussions internally as well where we also felt that naming the tool "gazel" or "glazel" would lose much of the meaning and pun around the original name, "glaze." |
Would you consider splitting the PR into 2-3 smaller ones, making it easier on me to review? |
@pmbethe09 @davidzchen FYI, when I named it "gazel" I thought about the following things:
|
Side note for the name gazel: https://en.wikipedia.org/wiki/Gazelle On Wed, Aug 3, 2016 at 1:50 PM Yuki Yugui Sonoda notifications@github.com
|
@yugui, thanks for the explanation, and @damienmg, that is a great suggestion! How about if we name the tool Gazelle? The main thing that makes me hesitant about "Gazel" is that it doesn't really resemble (or sound like) a real word in any language that I am aware of. On the other hand, Gazelle would not only fulfill the criteria that @yugui listed above but it is also a real word and adds the impression that it is fast. :) Another point that I just realized: while gazel is, in fact, the archaic form of gazelle, I think that if we want to go this route, it would be better to go with the name "gazelle" since the old form, "gazel," would likely introduce more confusion about how "gazel" and "bazel" are pronounced (since "bazel" is meant to be pronounced like "basil"). |
@davidzchen Sounds good. Let's go with "Gazelle". |
thanks for doing this, and Gazelle is a lovely name :) |
Supported:
load
golang build rulesgo_prefix
if necessarygo_library
,go_binary
,go_test
rulesBUILD
files, show diff or just prints the result into stdoutTo be supported later:
visibility
BUILD
filesgo_vendor
rule which automatically applies Gazel to external repositories, as discussed in Support specifying custom package name #16 (comment)Gazel should be considered experimental. It is possible that this is replaced with glaze when Google opensources it.
c.f. #15