mvdan / sh

A shell parser, formatter, and interpreter with bash support; includes shfmt
https://pkg.go.dev/mvdan.cc/sh/v3
BSD 3-Clause "New" or "Revised" License
7.14k stars 338 forks source link

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

Open lollipopman opened 2 years ago

lollipopman commented 2 years ago

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 commented 2 years ago

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?

mvdan commented 2 years ago

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.

lollipopman commented 2 years ago

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?

mvdan commented 2 years ago

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.

lollipopman commented 2 years ago

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.

mvdan commented 1 year ago

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

lollipopman commented 1 year ago

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