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

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

Open
wants to merge 1 commit into
base: master
Choose a base branch
from

Conversation

lollipopman
Copy link
Contributor

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?

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
Copy link
Contributor Author

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
Copy link
Owner

mvdan commented May 23, 2022

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
Copy link
Contributor Author

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
Copy link
Owner

mvdan commented Jun 6, 2022

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
Copy link
Contributor Author

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
Copy link
Owner

mvdan commented Apr 8, 2023

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

@lollipopman
Copy link
Contributor Author

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

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants