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

Make line separator customisable #31

Merged
merged 2 commits into from
Dec 10, 2019

Conversation

LeonB
Copy link
Contributor

@LeonB LeonB commented Dec 2, 2019

No description provided.

Copy link
Owner

@ianlopshire ianlopshire left a comment

Choose a reason for hiding this comment

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

@LeonB,

Thanks for the PR! I'd be happy to merge this after a couple of changes. The encoder implementation of this can be found in #29. I'd like the implementation to match as closely as possible.

decode.go Outdated
@@ -21,7 +21,8 @@ func Unmarshal(data []byte, v interface{}) error {

// A Decoder reads and decodes fixed width data from an input stream.
type Decoder struct {
data *bufio.Reader
scanner *bufio.Scanner
separator string
Copy link
Owner

Choose a reason for hiding this comment

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

Suggested change
separator string
lineTerminator []byte

decode.go Outdated
line, err := d.data.ReadString('\n')
if err != nil && err != io.EOF {
return err, false
func (d *Decoder) SetSeparator(separator string) {
Copy link
Owner

Choose a reason for hiding this comment

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

Suggested change
func (d *Decoder) SetSeparator(separator string) {
func (d *Decoder) SetLineTerminator(lineTerminator []byte) {

decode.go Outdated
d.separator = separator
}

func (d Decoder) Separator() string {
Copy link
Owner

Choose a reason for hiding this comment

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

I don't see any reason this should be exported. Also is there any reason it is not a pointer receiver e.g. func (d *Decoder)?

decode.go Outdated
return "\n"
}

func (d *Decoder) Scan(data []byte, atEOF bool) (advance int, token []byte, err error) {
Copy link
Owner

Choose a reason for hiding this comment

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

Why implement the custom scan function? I think data.ReadString(d.lineTerminator) would work fine.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

@ianlopshire ReadString only takes a single byte as argument: https://golang.org/pkg/bufio/#Reader.ReadString

So in the case of "\r\n" this wouldn't work

@ianlopshire ianlopshire self-assigned this Dec 2, 2019
@LeonB LeonB force-pushed the line-separator branch 2 times, most recently from 9b59077 to 0998b83 Compare December 10, 2019 13:16
@LeonB
Copy link
Contributor Author

LeonB commented Dec 10, 2019

@ianlopshire I've added your suggestions. The suggestion about using ReadString instead of Scan wouldn't work I think because ReadString only takes a single bytes as an argument: https://golang.org/pkg/bufio/#Reader.ReadString

So in the case of \r\n that wouldn't work.

I'm happy to edit the implementation but this was the best I could come up with besides a custom implementation of a ReadString like function.

@ianlopshire ianlopshire merged commit 7b141ba into ianlopshire:master Dec 10, 2019
@ianlopshire
Copy link
Owner

@LeonB, thanks for making the changes! merged!

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