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

proposal: x/tools: move cmd/internal/edit to a public package #29824

Closed
josharian opened this issue Jan 18, 2019 · 24 comments
Closed

proposal: x/tools: move cmd/internal/edit to a public package #29824

josharian opened this issue Jan 18, 2019 · 24 comments
Labels
FrozenDueToAge Proposal Tools This label describes issues relating to any tools in the x/tools repository.
Milestone

Comments

@josharian
Copy link
Contributor

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 of cmd/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.

@gopherbot gopherbot added this to the Unreleased milestone Jan 18, 2019
@josharian josharian added the NeedsDecision Feedback is required from experts, contributors, and/or the community before a change can be made. label Jan 18, 2019
@mvdan
Copy link
Member

mvdan commented Jan 18, 2019

/cc @rogpeppe

@josharian
Copy link
Contributor Author

Ping. I just made yet another copy of this package.

Who is the decision maker for this?

@thepudds
Copy link
Contributor

thepudds commented May 8, 2019

Hi @mvdan, I see you commented here earlier. Do you know who the decision maker is for this?

@mvdan
Copy link
Member

mvdan commented May 8, 2019

@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.

@ianlancetaylor ianlancetaylor modified the milestones: Unreleased, Go1.13 May 8, 2019
@ianlancetaylor
Copy link
Contributor

I changed the milestone so that it will be looked at sooner rather than later.

@andybons andybons modified the milestones: Go1.13, Go1.14 Jul 8, 2019
@gopherbot gopherbot added the Tools This label describes issues relating to any tools in the x/tools repository. label Sep 12, 2019
@rsc rsc modified the milestones: Go1.14, Backlog Oct 9, 2019
@josharian
Copy link
Contributor Author

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.

I changed the milestone so that it will be looked at sooner rather than later.

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.

@ianlancetaylor ianlancetaylor changed the title x/tools: move cmd/internal/edit to a public package proposal: x/tools: move cmd/internal/edit to a public package Nov 20, 2021
@gopherbot gopherbot added Proposal and removed NeedsDecision Feedback is required from experts, contributors, and/or the community before a change can be made. labels Nov 20, 2021
@ianlancetaylor ianlancetaylor modified the milestones: Backlog, Proposal Nov 20, 2021
@ianlancetaylor ianlancetaylor added this to Incoming in Proposals (old) Nov 20, 2021
@ianlancetaylor
Copy link
Contributor

What should we call the public package? x/tools/edit?

@josharian
Copy link
Contributor Author

x/tools/edit works for me. The package name works well already (e.g. edit.Buffer), I don't see any obvious existing subdir to put it in, and I can't think of any obvious future subdir that it belongs to.

@findleyr
Copy link
Contributor

This package is small and useful. Adding it to x/tools makes sense to me.

@rsc
Copy link
Contributor

rsc commented Dec 1, 2021

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.

@rsc
Copy link
Contributor

rsc commented Dec 1, 2021

This proposal has been added to the active column of the proposals project
and will now be reviewed at the weekly proposal review meetings.
— rsc for the proposal review group

@rsc rsc moved this from Incoming to Active in Proposals (old) Dec 1, 2021
@josharian
Copy link
Contributor Author

Why do you keep making new copies of this package instead of making one public copy, like rogpeppe did with txtar? :-)

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.

@gopherbot
Copy link

Change https://golang.org/cl/370880 mentions this issue: edit: add new package for applying edits to byte streams

@rsc
Copy link
Contributor

rsc commented Dec 10, 2021

Looked over the API.
It has some 'panic on usage mistake' that probably don't belong in a public package.
I revised the API a bit and sent CL 370880.

package edit // import "golang.org/x/tools/edit"

Package edit implements buffered position-based editing of byte slices.

TYPES

type Buffer struct {
	// Has unexported fields.
}
    A Buffer is a queue of edits to apply to a given byte slice. The queued
    edits must not overlap.

func NewBuffer(data []byte) *Buffer
    NewBuffer returns a new buffer to accumulate changes to an initial data
    slice. The returned buffer maintains a reference to the data, so the caller
    must ensure the data is not modified until after the Buffer is done being
    used.

func (b *Buffer) Apply() ([]byte, error)
    Apply returns a new byte slice containing the original data with the queued
    edits applied. It returns an error if any of the queued edits overlap or
    refer to invalid string positions.

func (b *Buffer) Delete(start, end int)

func (b *Buffer) Insert(pos int, new string)

func (b *Buffer) Replace(start, end int, new string)

@josharian
Copy link
Contributor Author

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.

@rsc
Copy link
Contributor

rsc commented Dec 10, 2021

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.

@rogpeppe
Copy link
Contributor

rogpeppe commented Dec 10, 2021

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.

@rsc
Copy link
Contributor

rsc commented Dec 14, 2021

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.

@rsc
Copy link
Contributor

rsc commented Jan 5, 2022

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?

@josharian
Copy link
Contributor Author

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.

@rsc
Copy link
Contributor

rsc commented Jan 19, 2022

My plan is to publish a copy under rsc.io.

@rsc
Copy link
Contributor

rsc commented Jan 19, 2022

Based on the discussion above, this proposal seems like a likely decline.
— rsc for the proposal review group

@rsc rsc moved this from Active to Likely Decline in Proposals (old) Jan 19, 2022
@rsc
Copy link
Contributor

rsc commented Jan 19, 2022

Published https://pkg.go.dev/rsc.io/edit@v1.0.0.

@rsc rsc moved this from Likely Decline to Declined in Proposals (old) Jan 27, 2022
@rsc
Copy link
Contributor

rsc commented Jan 27, 2022

No change in consensus, so declined.
— rsc for the proposal review group

@rsc rsc closed this as completed Jan 27, 2022
@golang golang locked and limited conversation to collaborators Jan 27, 2023
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
FrozenDueToAge Proposal Tools This label describes issues relating to any tools in the x/tools repository.
Projects
No open projects
Development

No branches or pull requests

9 participants