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
proposal: x/tools: move cmd/internal/edit to a public package #29824
Comments
/cc @rogpeppe |
Ping. I just made yet another copy of this package. Who is the decision maker for this? |
Hi @mvdan, I see you commented here earlier. Do you know who the decision maker is for this? |
@rsc added the package and @ianlancetaylor approved it. This is used for cmd/cover, so I guess @bcmills could also make a decision here. I'm not sure. |
I changed the milestone so that it will be looked at sooner rather than later. |
We just made yet another copy of this package. And I have a minor improvement I'd like to upstream. It'd be nice if we could move this to x/tools so I can send a CL, and then vendor it back in Go 1.19.
Looks like that didn't work. :( The work here is minimal (code movement), and I'm happy to do it. I'm just looking for a green light. |
What should we call the public package? x/tools/edit? |
|
This package is small and useful. Adding it to x/tools makes sense to me. |
Why do you keep making new copies of this package instead of making one public copy, like rogpeppe did with txtar? :-) I will take a look at the API and see whether I'm happy with it being exported. |
This proposal has been added to the active column of the proposals project |
Honest answer: Because I believed that the request was trivial enough that it would be green-lighted quickly, and I didn't want to create a competing package. |
Change https://golang.org/cl/370880 mentions this issue: |
Looked over the API.
|
LGTM, Russ. Thanks. Incidentally, the "minor improvement" I have been carrying around that I might attempt to upstream is josharian@a913612. It doesn't impact the API, and looks like it would apply obviously/cleanly to the new code. |
That change does seem reasonable. I originally used this package in rf, but it ended up wrapped in a more idiomatic API for that setting, with different ways to specify positions, and it ended up not pulling its weight. I worry just a little bit about that here too, but it won't bother anyone for it to exist. The other possible place for it would be bytes.Editor (arguably a nicer name), but it doesn't seem important enough for the standard library. |
in passing, I'm sure it fitted the use case at the time, but it perhaps feels a bit odd that it's created with a byte slice and produces a byte slice but the operations are in terms of strings not byte slices. |
I agree about that []byte vs string issue, which is another concern I have. If you use []byte you have to say that the buffers can't be reused, or you have to say they can be and make copies yourself. It's also also somewhat concerning that when I went to use this code in rf I first wrapped it and then ended up replacing it entirely, because in a larger program the API I needed was sufficiently different that the code wasn't pulling its weight. It really is a fairly trivial loop when it comes down to it. |
I'm still a bit unsure that the API weight here is enough to be worth having in a public x/tools package. @josharian, which flavor of the API were you looking for? |
Every context where I've used this, the performance go/types dwarfs the costs of string/[]byte conversions. I would be happy with either flavor of API. I'm also happy to just hard fork the package. Whatev. |
My plan is to publish a copy under rsc.io. |
Based on the discussion above, this proposal seems like a likely decline. |
Published https://pkg.go.dev/rsc.io/edit@v1.0.0. |
No change in consensus, so declined. |
cmd/internal/edit
is incredibly useful when modifying code, particular as compared to manually constructing an AST for go/printer to print. That's not surprising; it is the reason that Russ wrote it. I have a half dozen copies ofcmd/internal/edit
floating around, so that I can use it tools that I write.Given its general utility when writing tools, I propose that we move cmd/internal/edit to x/tools, and then vendor it back into core.
The text was updated successfully, but these errors were encountered: