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

fix: Parse prompt input JSON using object or array chars #538

Merged
merged 1 commit into from
Mar 24, 2022

Conversation

kylecarbs
Copy link
Member

Fixes #492. There is no more single-quote parsing, and instead we use a JSON decoder for multiline values. This is a much better UX!

@kylecarbs kylecarbs requested a review from coadler March 23, 2022 17:15
@kylecarbs kylecarbs self-assigned this Mar 23, 2022
@codecov
Copy link

codecov bot commented Mar 23, 2022

Codecov Report

Merging #538 (60ad570) into main (305b67c) will increase coverage by 0.43%.
The diff coverage is 100.00%.

@@            Coverage Diff             @@
##             main     #538      +/-   ##
==========================================
+ Coverage   62.65%   63.08%   +0.43%     
==========================================
  Files         194      194              
  Lines       10827    10837      +10     
  Branches       85       85              
==========================================
+ Hits         6784     6837      +53     
+ Misses       3302     3269      -33     
+ Partials      741      731      -10     
Flag Coverage Δ
unittest-go- 61.99% <100.00%> (+0.39%) ⬆️
unittest-go-macos-latest 57.55% <100.00%> (+0.11%) ⬆️
unittest-go-ubuntu-latest 60.90% <100.00%> (+0.16%) ⬆️
unittest-go-windows-2022 56.92% <100.00%> (+0.04%) ⬆️
unittest-js 63.32% <ø> (ø)
Impacted Files Coverage Δ
cli/cliui/prompt.go 78.26% <100.00%> (+3.68%) ⬆️
cli/cliui/job.go 64.86% <0.00%> (-1.81%) ⬇️
provisionerd/provisionerd.go 80.49% <0.00%> (+1.31%) ⬆️
coderd/workspaceresources.go 58.98% <0.00%> (+1.38%) ⬆️
peer/conn.go 79.18% <0.00%> (+1.77%) ⬆️
provisioner/echo/serve.go 59.20% <0.00%> (+2.40%) ⬆️
coderd/provisionerdaemons.go 62.18% <0.00%> (+3.57%) ⬆️
provisionersdk/transport.go 85.10% <0.00%> (+6.38%) ⬆️

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update 305b67c...60ad570. Read the comment docs.

Fixes #492. There is no more single-quote parsing, and instead we use a JSON decoder for multiline values. This is a much better UX!
Comment on lines +55 to +61
pipeReader, pipeWriter := io.Pipe()
defer pipeWriter.Close()
defer pipeReader.Close()
go func() {
_, _ = pipeWriter.Write([]byte(line))
_, _ = reader.WriteTo(pipeWriter)
}()
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I'm a bit confused about the purpose of the pipe here. Couldn't you just create a new bytes.Buffer from line and call reader.WriteTo(newBuf)?

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

https://pkg.go.dev/bufio#Reader.Peek would work well here too

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Nah, bytes.Buffer will EOF if read, which we don't want. I had that initially too!

I was going to change the reader to peek, but it's an unknown amount of bytes until a newline, so it felt a bit off!

Going to merge, but if the above doesn't make sense just LMK!

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Interesting. Makes sense to me!

@kylecarbs kylecarbs merged commit 99ece25 into main Mar 24, 2022
@kylecarbs kylecarbs deleted the multilineprompt branch March 24, 2022 01:12
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.

service account key must be one line (and doesn't require quotes)
2 participants