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.

Sign up for GitHub

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

Jump to bottom

Add new line to cmdio JSON rendering #443

Merged
merged 3 commits into from
Jun 8, 2023
Merged

Add new line to cmdio JSON rendering #443

Changes from all commits
Commits
Show all changes
3 commits
Select commit Hold shift + click to select a range
5128ada
Add new line to cmdio JSON rendering
shreyas-goenka Jun 5, 2023
31934f7
Merge remote-tracking branch 'origin' into new-line-on-render
shreyas-goenka Jun 5, 2023
594316d
Merge branch 'main' into new-line-on-render
pietern Jun 8, 2023
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
4 changes: 4 additions & 0 deletions libs/cmdio/render.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -50,6 +50,10 @@ func renderJson(w io.Writer, v any) error {
return err
}
_, err = w.Write(pretty)
if err != nil {
return err
}
_, err = w.Write([]byte("\r\n"))
Copy link
Contributor

Choose a reason for hiding this comment

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

Please stick to \n only. On Windows folks seem to get by with \n alone as well.

Copy link
Contributor Author

@shreyas-goenka shreyas-goenka Jun 6, 2023 •
edited
Loading

Choose a reason for hiding this comment

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

That seems to be the case when using \n with println. I am not so sure about io writer though. We do the same for sync progress events:

cli/cmd/sync/output.go

Line 49 in be10ff9

bw.WriteString("\r\n")

Maybe it's worthwhile testing it out on windows?

Copy link
Contributor

Choose a reason for hiding this comment

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

yep, please check it on windows

Copy link
Contributor

@pietern pietern Jun 8, 2023 •
edited
Loading

Choose a reason for hiding this comment

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

fmt.Println goes through an io.Writer, the two operate at different levels.

Please file a follow up to check out if \n alone is good enough. Use of \r\n on Unix systems could be problematic so I prefer using \n everywhere. If it's good enough for the Go stdlib it is good enough for us.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Followup filed at #451

return err
}

Expand Down