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

Add proof of concept for T.Output() #1

Draft
wants to merge 28 commits into
base: master
Choose a base branch
from
Draft

Conversation

suntala
Copy link
Owner

@suntala suntala commented Sep 26, 2023

return OutputWriter{c}
}

type OutputWriter struct {
Copy link

Choose a reason for hiding this comment

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

unexport

parent.mu.Lock()
defer parent.mu.Unlock()
if !parent.done {
parent.output = append(parent.output, parent.decorate(str, depth+1)...)
Copy link

Choose a reason for hiding this comment

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

This Write method is at a lower level than logDepth. It isn't responsible for adding line info or a final newline, so it shouldn't call decorate. It does need to handle indentation, though. I'm not sure where that happens.

@suntala suntala force-pushed the suntala/t-output-poc branch from f8a90ab to 61379e3 Compare October 20, 2023 19:13
@suntala suntala closed this Jan 15, 2025
@suntala suntala reopened this Jan 25, 2025
suntala and others added 4 commits January 25, 2025 10:39
Co-authored-by: Aleks Fazlieva <[email protected]>
Co-authored-by: Aleks Fazlieva <[email protected]>
Co-authored-by: Aleks Fazlieva <[email protected]>
@suntala suntala force-pushed the suntala/t-output-poc branch from 3fc09b7 to c949982 Compare January 30, 2025 17:35
@jba
Copy link

jba commented Jan 31, 2025

I took another careful look at this. I do think you're on the right track: outputWriter.Write should buffer lines, direct the output to the right place (this test, parent test, etc.) and prepend four spaces. Here is what I think needs to be done:

  • Move to gerrit so we can have a better review experience, others can comment, and your CL will be linked from the proposal issue. Gerrit also allows having multiple CLs stacked on top of one another, which I think will be valuable. See https://golang.org/doc/contribute.html.

  • My gut tells me that you should have a function outputWriter.writeLine([]byte). Ultimately all output is a line at a time, so that will be a useful building block. I think you might want it to rewrite the exising code (see below). If you want the arg to be string instead of []byte, that's fine to start. I'm not sure if the arg should include the newline or not; you decide.

  • Rewrite logDepth and decorate to use outputWriter. In fact, I think the first CL should do that rewrite and make sure all tests pass. It wouldn't even expose t.Output. It would just be an internal change. I think that would increase confidence that you're on the right track, and you could temporarily defer handling various edge cases.

  • The second CL would export t.Output and add tests for things that can't happen with t.Log, which I think all have to do with partial lines. For example, what happens if you have an incomplete line and the test ends? You'll need to flush the buffer (adding a final newline).

@suntala
Copy link
Owner Author

suntala commented Feb 3, 2025

@jba thank you so much for the guidance. Doing this in two CLs, with the first one being a rewrite of the log/logDepth is a good idea. Since your message, we did the refactor and got tests working. Next steps are going to be cleaning up the code and moving to gerrit. We're aiming to get them done in the next couple of days.

@jba
Copy link

jba commented Feb 3, 2025

Sounds good. I'll hold off reviewing until it's on gerrit.

@suntala suntala force-pushed the suntala/t-output-poc branch from b98c1b4 to 7648014 Compare February 4, 2025 22:03
@suntala suntala force-pushed the suntala/t-output-poc branch 4 times, most recently from d865109 to 820826a Compare February 5, 2025 10:21
Co-authored-by: Aleks Fazlieva <[email protected]>
@suntala suntala force-pushed the suntala/t-output-poc branch from 820826a to 00014da Compare February 5, 2025 10:24
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