Move most of gazelle main package into a subpackage #1292
Conversation
| } | ||
| } | ||
| return false | ||
| } |
There was a problem hiding this comment.
this is a 99% move/rename, but unfortunately github isn't showing this well since the old gazelle.go is still there and replaced.
There was a problem hiding this comment.
I could probably rename cmd/gazelle/gazelle.go -> cmd/gazelle/gazelle-cmd.go to get github to show the correct diff.
|
I think we should be very very careful about extending our public interface. It sounds to me like the intent here is to only expose |
It's just those two. Funny enough, @sluongng and I went back and forth a bit on exactly how that should be formulated. |
|
How do we get this landed? 😺 |
|
Annnd.... now this PR is stale, since it's a month old. I'd love to update this, but can we first agree on whether or not this can be landed? Or what changes are needed to land it? |
|
I'd like to see development on this PR resume. Maintaining a separate copy of cmd/gazelle in https://github.com/stackb/rules_proto/tree/master/cmd/gazelle is a pain. |
|
@achew22 I think this PR is currently pending your review? Do you have any objection merging this as-is or suggestion on how to improve this? We also have to patch Gazelle to make things work on our end, some having this PR merged would be very nice. |
|
I suspect the age of the PR will require someone making sure all the changes that have occurred in cmd/gazelle since July 2022 are faithfully propagated to this one. I'd be hesitant to merge it as-is without careful review of that even if checks are passing. |
|
Indeed, this branch is full of conflicts. I'll basically need to re-do this PR. But I also don't want to waste time if this can't land. Basically, I'm waiting on someone with the power to merge PRs to actively engage on this and offer a path to getting this merged. |
|
FWIW I'm +1 on accepting this. @achew22 I believe your question was answered previously, do you have any remaining concerns? |
|
My concerns mostly revolve around that this isn't really an interface ever designed to be part of a v1 API. As evidenced by the buzz around this PR, I'm inclined to believe that if we expose this API, people will come to depend on it in it's current form. I would love to see a refactor of this logic to allow the runner to be embedded into other applications, but I would like API design to be something that is at least considered instead of "just use the old API because it is there". |
|
If it makes a difference, this repo being on v0.31.0 (rather than say, 1.0) means that there's no guarantee of API compatibility, at least in the Go world. I personally just want to be able to use this repo as a Go module, knowing that when I update to a newer gazelle it might break my client code that uses it. |
What type of PR is this?
Feature
What package or component does this PR mostly affect?
cmd/gazelle
What does this PR do? Why is it needed?
For people who are extending gazelle with other languages, IDE development is a chore. There's no easy way to produce a working gazelle binary with a desired set of languages configured. Allowing the implementation of
/cmd/gazelleto be accessible enables IDE development and debug without needing bazel code generation.Which issues(s) does this PR fix?
Fixes #1291