search

google / starlark-go

Starlark in Go: the Starlark configuration language, implemented in Go
BSD 3-Clause "New" or "Revised" License
2.26k stars 204 forks source link

Amend welcome banner and final println #413

Closed tauraamui closed 1 year ago

tauraamui commented 1 year ago

Context:

When providing programs via stdin the REPL welcome banner and trailing newlines are included regardless. Eg., echo '2 + 2' | starlark outputs:

Welcome to Starlark (go.starlark.net)
4

This fix removes these.

google-cla[bot] commented 1 year ago

Thanks for your pull request! It looks like this may be your first contribution to a Google open source project. Before we can look at your pull request, you'll need to sign a Contributor License Agreement (CLA).

View this failed invocation of the CLA check for more information.

For the most up to date status, view the checks section at the bottom of the pull request.

adonovan commented 1 year ago

Hi Adam, thanks for the PR. I wont be able to get to this till mid-August, but the condition you want to check is whether stdin is connected to a terminal. You can use this function: https://pkg.go.dev/golang.org/x/term#IsTerminal

cheers alan

On Tue, Jul 19, 2022 at 9:14 PM Adam Stringer @.***> wrote:

Context:

When providing programs via stdin the REPL welcome banner and trailing newlines are included regardless. Eg., echo '2 + 2' | starlark outputs:

Welcome to Starlark (go.starlark.net) 4

This fix removes these.

You can view, comment on, or merge this pull request online at:

https://github.com/google/starlark-go/pull/413 Commit Summary

File Changes

(2 files https://github.com/google/starlark-go/pull/413/files)

Patch Links:

— Reply to this email directly, view it on GitHub https://github.com/google/starlark-go/pull/413, or unsubscribe https://github.com/notifications/unsubscribe-auth/ABLFMPYDX3YMK43HKVJVWXLVU35BPANCNFSM54BBMUFQ . You are receiving this because you are subscribed to this thread.Message ID: @.***>

tauraamui commented 1 year ago

Hi Adam, thanks for the PR. I wont be able to get to this till mid-August, but the condition you want to check is whether stdin is connected to a terminal. You can use this function: https://pkg.go.dev/golang.org/x/term#IsTerminal cheers alan

Hi Alan,

I understand about the delay, there was another PR I was going to raise which is to have an optional method of loading programs from a given fs.FS, it's something I was hoping to use for work tomorrow, I guess in the meantime I could use my fork. Anyway, back on topic, so I've tried this out:

fromStdin := term.IsTerminal(int(os.Stdin.Fd()))
println(fromStdin)

results in false, so not sure if that's just me, have I made a mistake here?

adonovan commented 1 year ago

That code looks fine. IsTerminal returns false when you feed the program from echo. Perhaps you need to negate it?

As for fs.FS, that’s an interesting idea, but the interpreter shouldn’t need any changes: it should be possible to accomplish this with an external helper function. Feel free to send a PR with what you have in mind.

On Tue, Jul 19, 2022 at 11:04 PM Adam Stringer @.***> wrote:

Hi Adam, thanks for the PR. I wont be able to get to this till mid-August, but the condition you want to check is whether stdin is connected to a terminal. You can use this function: https://pkg.go.dev/golang.org/x/term#IsTerminal cheers alan … <#m_30823998468614302_m7943616756138365626> On Tue, Jul 19, 2022 at 9:14 PM Adam Stringer @.> wrote: Context: When providing programs via stdin the REPL welcome banner and trailing newlines are included regardless. Eg., echo '2 + 2' | starlark outputs: Welcome to Starlark (go.starlark.net http://go.starlark.net) 4 This fix removes these. ------------------------------ You can view, comment on, or merge this pull request online at: #413 https://github.com/google/starlark-go/pull/413 Commit Summary - f9e34c3 https://github.com/google/starlark-go/commit/f9e34c3f0f62890e9c7a7cf66bea20cfeaf3f51e <f9e34c3 https://github.com/google/starlark-go/pull/413/commits/f9e34c3f0f62890e9c7a7cf66bea20cfeaf3f51e> Amend welcome banner and final println File Changes (2 files https://github.com/google/starlark-go/pull/413/files https://github.com/google/starlark-go/pull/413/files) - M cmd/starlark/starlark.go https://github.com/google/starlark-go/pull/413/files#diff-b95b991cd232a829171e1158f2b49adee6bde571705534ac5501c8e81f7d90b8 https://github.com/google/starlark-go/pull/413/files#diff-b95b991cd232a829171e1158f2b49adee6bde571705534ac5501c8e81f7d90b8 (20) - M repl/repl.go https://github.com/google/starlark-go/pull/413/files#diff-f3738eaa02e99234edda29447e81abd0c95e16bed33ee6a499379d8b34219373 https://github.com/google/starlark-go/pull/413/files#diff-f3738eaa02e99234edda29447e81abd0c95e16bed33ee6a499379d8b34219373 (1) Patch Links: - https://github.com/google/starlark-go/pull/413.patch https://github.com/google/starlark-go/pull/413.patch - https://github.com/google/starlark-go/pull/413.diff https://github.com/google/starlark-go/pull/413.diff — Reply to this email directly, view it on GitHub <#413 https://github.com/google/starlark-go/pull/413>, or unsubscribe https://github.com/notifications/unsubscribe-auth/ABLFMPYDX3YMK43HKVJVWXLVU35BPANCNFSM54BBMUFQ https://github.com/notifications/unsubscribe-auth/ABLFMPYDX3YMK43HKVJVWXLVU35BPANCNFSM54BBMUFQ . You are receiving this because you are subscribed to this thread.Message ID: @.>

Hi Alan,

I understand about the delay, there was another PR I was going to raise which is to have an optional method of loading programs from a given fs.FS, it's something I was hoping to use for work tomorrow, I guess in the meantime I could use my fork. Anyway, back on topic, so I've tried this out:

— Reply to this email directly, view it on GitHub https://github.com/google/starlark-go/pull/413#issuecomment-1189548105, or unsubscribe https://github.com/notifications/unsubscribe-auth/ABLFMP6LFGQB5SFVMRPWSPLVU4J5LANCNFSM54BBMUFQ . You are receiving this because you commented.Message ID: @.***>

tauraamui commented 1 year ago

That code looks fine. IsTerminal returns false when you feed the program from echo. Perhaps you need to negate it? As for fs.FS, that’s an interesting idea, but the interpreter shouldn’t need any changes: it should be possible to accomplish this with an external helper function. Feel free to send a PR with what you have in mind.

"IsTerminal returns false when you feed the program from echo

Ah I see, that's interesting, I imagine it's false if the data comes from any pipe at all, echo or not? I'll make the change with that in mind.

it should be possible to accomplish this with an external helper function

I did originally think that, but then my original read of the scanner's readSource function led me to believe that it doesn't have the ability to read from a provided reader, just checked it again and yay it does! Thanks! I'll hold off on the PR for now so I can get some "real" work done.