-
Notifications
You must be signed in to change notification settings - Fork 387
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
New command: migrate-deps #47
Conversation
With this change, Gazelle supports a new command: migrate-deps. This can be used to migrate dependencies from a vendoring lock file (only dep's Gopkg.lock for now, but more to come) to go_repository rules in WORKSPACE. Related bazel-contrib#29
merger/merger.go
Outdated
m = &nameMatcher{kind, name(c)} | ||
} | ||
// match looks for the matching CallExpr in stmts. If a rule with a matching | ||
// "name" attribute is found, and the kind matches, the rule and its index |
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.
the other way around, index then rule
merger/merger.go
Outdated
// are returned. If a rule with a matching name is found but the kind does not | ||
// match, the index of the rule and nil are returned. If no match is found, | ||
// -1 and nil are returned. | ||
func match(stmts []bf.Expr, x *bf.CallExpr) (int, *bf.CallExpr) { |
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.
The api is not great, you can't do _, rule = match(...) when you don't care about type.
Not needed to fix for this PR (it's a private rarely called function) but consider changing to something that maybe returns index, rule, bool where index and rule are both always set or not together, and the final bool indicates a full or partial match (or something)
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, that seems sensible.
This function will need to change in the future anyway, since we'll need to match on more attributes (e.g., importpath).
repos/repo.go
Outdated
func (s byName) Swap(i, j int) { s[i], s[j] = s[j], s[i] } | ||
|
||
var lockFileParsers = map[string]func(string) ([]repo, error){ | ||
"Gopkg.lock": importRepoRulesDep, |
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.
I think you should add an extra step of indirection in here.
Have a function that peeks at a file to guess the "type" and returns a handler for that type.
The import them becomes a method on the returned object.
At the moment the only peeking is checking the base name, but you could also imagine looking inside the file for some magic to disambiguate. Also gives you a useful value to hang other things off in the future.
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
With this change, Gazelle supports a new command: migrate-deps. This
can be used to migrate dependencies from a vendoring lock file (only
dep's Gopkg.lock for now, but more to come) to go_repository rules in
WORKSPACE.
Related #29