sh icon indicating copy to clipboard operation
sh copied to clipboard

read builtin: implement -a, -s, -n, -N and -d

Open lollipopman opened this issue 3 years ago • 7 comments

This implements some of the commonly used options for the read builtin, with the notable exception of -t.

Questions:

  1. I don't love the duplication of some of the x/term code, but I couldn't find another option?
  2. At present -s is completely untested, any suggestions on where or how to test code, which relies on interacting with a terminal?

lollipopman avatar May 23 '22 14:05 lollipopman

So it looks like we would need most of the platform specific code from https://github.com/golang/term to make this work properly, since altering the terminal is platform specific. @mvdan do you have any thoughts on the best way to utilize that code, any other options other than copying wholesale?

lollipopman avatar May 23 '22 15:05 lollipopman

Do we just need to copy the code to access unexported APIs, or do we need to copy bits of code to tweak them? If it's just the former, I would consider something like https://pkg.go.dev/golang.org/x/tools/cmd/bundle.

mvdan avatar May 23 '22 19:05 mvdan

Do we just need to copy the code to access unexported APIs, or do we need to copy bits of code to tweak them? If it's just the former, I would consider something like https://pkg.go.dev/golang.org/x/tools/cmd/bundle.

At present only the former, namely:

// These platform dependent definitions:
const ioctlReadTermios = unix.TCGETS
const ioctlWriteTermios = unix.TCSETS

Bundle looks neat, but I don't think it would work, as it has this caveat: "must not use conditional file compilation", which I assume won't work with build tags such as //go:build darwin || dragonfly || freebsd || netbsd || openbsd inside term's source?

lollipopman avatar May 23 '22 20:05 lollipopman

Ah, that's unfortunate. The bundle tool outputs a single filename, so I guess it's only logical that it has that limitation given that it's a simple tool.

What about adding the API in x/term that we need here? It could take weeks or even months for such an API proposal to be accepted and merged, but at least we wouldn't be copy-pasting chunks of code.

For now, though, manual copying seems fine. I would just ask that you document it well, so that keeping the code up to date is reasonably easy in the future.

mvdan avatar Jun 06 '22 11:06 mvdan

What about adding the API in x/term that we need here? It could take weeks or even months for such an API proposal to be accepted and merged, but at least we wouldn't be copy-pasting chunks of code.

Yeah, I think that is worthwhile, I'll try to get a pull request against x/term for those folks to peruse.

For now, though, manual copying seems fine. I would just ask that you document it well, so that keeping the code up to date is reasonably easy in the future.

sounds good, I'll take that route after making the aforementioned pull request.

lollipopman avatar Jun 06 '22 19:06 lollipopman

Friendly ping, do you intend to get back to this? It's gained conflicts since we last looked at it :)

mvdan avatar Apr 08 '23 14:04 mvdan

@mvdan thanks for the reminder, I lost steam on getting patches created for x/term, but you have re-inspired me!

lollipopman avatar Apr 10 '23 18:04 lollipopman