Skip to content

Add a new method Flush to reuse writer #18

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
Merged
merged 4 commits into from
Oct 21, 2020
Merged

Add a new method Flush to reuse writer #18

merged 4 commits into from
Oct 21, 2020

Conversation

kiyonlin
Copy link
Contributor

@kiyonlin kiyonlin commented Oct 20, 2020 •
edited
Loading

This PR add a new method Flush to reuse writer. Here is the code snippet

package main

import (
	"fmt"
	"io"

	"github.com/muesli/reflow/padding"
)

func main() {
	f := padding.NewWriter(6, func(w io.Writer) {
		_, _ = w.Write([]byte("."))
	})
	f.Write([]byte("foo\nbar"))
	f.Flush()
	// foo...\nbar...
	fmt.Println(f.String())

	f.Write([]byte("bar\nbaz"))
	f.Flush()
	// bar...\nbaz...
	fmt.Println(f.String())
}

Further more, when writer closed, it shouldn't be used again according to #13(comment).

Btw I'm not sure the descriptions about Flush and Close conform to your standard. So if you any ideas, please let me know.

muesli
muesli reviewed Oct 21, 2020
View reviewed changes
padding/padding.go Outdated
if w.lineLen == 0 {
return nil
// Close will finish the padding operation and then closes the writer.
// Notice that the writer will never be wrote again after it closed.
Copy link
Owner

Choose a reason for hiding this comment

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

Typo/grammar: "Notice that the writer can never be written to again once it has been closed."

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Thanks for the correction!

padding/padding_test.go Outdated
t.Error(err)
}

f.closed = true
Copy link
Owner

Choose a reason for hiding this comment

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

Why do you manually set this? Shouldn't closed already be true after f.Close()?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

No, closed won't be set to true if error occurs.

@muesli
Copy link
Owner

muesli commented Oct 21, 2020

I must admit this added a bit more overhead than initially anticipated. All reasonable changes, but I wonder if it's really worth adding it, as it certainly hinders readability & understandability a bit. Maybe we could/should abstract the generic Writer caching away, especially if we want to introduce the same API pattern to the other packages in reflow.

@kiyonlin
Copy link
Contributor Author

Thanks for the feedback!

In my opinion, all writers in reflow is kind of like bufio.Writer. To be honest I think they shouldn't have Close method(can be deprecated) and Flush is more reasonable.

Perhaps the most commonly used methods are String and Bytes. The writers they use internally can be reused.

@kiyonlin
Copy link
Contributor Author

kiyonlin commented Oct 21, 2020 •
edited
Loading

Btw if we can combine f.Flush with f.String to f.FlushString (so does Bytes), we can remove the cache buffer.

But it is bigger changes haha. It's just an idea and up to you 😄 .

@muesli
Copy link
Owner

muesli commented Oct 21, 2020

Good point, indeed. Maybe we should follow that API. On the other side I always liked this being compatible with an io.WriteCloser. There's no such thing for finalizing an operation with a Flush call (that I'm aware of).

@muesli
Copy link
Owner

muesli commented Oct 21, 2020

Maybe we should make these structures non-closable (internally), and instead have Close() be an alias for Flush(), so we remain compatible with the io.WriteCloser interface and don't break backwards-compatibility either.

@kiyonlin
Copy link
Contributor Author

kiyonlin commented Oct 21, 2020 •
edited
Loading

Maybe we should make these structures non-closable (internally), and instead have Close() be an alias for Flush(), so we remain compatible with the io.WriteCloser interface and don't break backwards-compatibility either.

That may be something like ioutil.NopCloser, right?

Oh as of go 1.16 will be io.NopCloser.

@kiyonlin
Copy link
Contributor Author

Updated. Please take a look @muesli

@muesli
Copy link
Owner

muesli commented Oct 21, 2020 •
edited
Loading

That may be something like ioutil.NopCloser, right?

Not quite, as the NopCloser wrapper really does nothing when Close gets called. Here we do actually need it to be called, so we can flush our buffers.

@kiyonlin
Copy link
Contributor Author

That may be something like ioutil.NopCloser, right?

Not quite, as the NopCloser wrapper really does nothing when Close gets called. Here we do actually need it to be called, so we can flush our buffers.

Got it. And other packages in reflow should have the same API pattern.

@muesli
Copy link
Owner

muesli commented Oct 21, 2020

Got it. And other packages in reflow should have the same API pattern.

Yeah I think we should keep them consistent.

@kiyonlin
Copy link
Contributor Author

Got it. And other packages in reflow should have the same API pattern.

Yeah I think we should keep them consistent.

I will fix them all after padding is perfect.

@muesli
Copy link
Owner

muesli commented Oct 21, 2020

I think this PR is looking good now! Thanks for all the iterations you went through. And once again: thank you for all your PRs!

@muesli muesli merged commit aa57fa9 into muesli:master Oct 21, 2020
@kiyonlin kiyonlin deleted the feature-flush branch October 21, 2020 08:10
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@muesli muesli muesli left review comments

Assignees
No one assigned
Labels
None yet
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
2 participants
@kiyonlin @muesli