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

Issue 6 buffer problem #9

Closed
wants to merge 2 commits into from

Conversation

asannikov
Copy link

An error nothing on stdout is occurred in case the token buffer is overflowed.
This error comes form this part of the code:

if !e.scanout.Scan() {
    fms[i].Err = fmt.Errorf("nothing on stdout")
    continue
}

And in core it comes from this part of the code:
https://github.com/golang/go/blob/master/src/bufio/scan.go#L193

@@ -64,6 +67,11 @@ func NewExiftool(opts ...func(*Exiftool) error) (*Exiftool, error) {
return &e, nil
}

// SetBufferSize changes the buffer size
func (e *Exiftool) SetBufferSize(size int) {
Copy link
Owner

Choose a reason for hiding this comment

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

I think that the buffer size specification should rather be specified as a functionnal options.

The thing that bothers me with this implementation (a method on the ExifTool struct) is that users could think that the buffer size could be set after ExifTool initiatialization, but it doesn't.

Could you please change your implementation to fit a functionnal option (func WithBufferSize func(*Exiftool) error { ... }) ? The NewExifTool function already accepts functionnal options.

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 mention unit-tests and the update of the documentation :)

@barasher
Copy link
Owner

@asannikov > Hi, I've reviewed your PR, I have some suggestions.
Thanks again !

@barasher barasher added this to the v1.2.0 milestone Aug 16, 2020
@barasher
Copy link
Owner

barasher commented Aug 16, 2020

@asannikov > do you want me to do the changes ?

@asannikov
Copy link
Author

asannikov commented Aug 16, 2020

@barasher

@asannikov > do you want me to do the changes ?

yes please. i do not really understand how to implement that in your way.
NewExiftool has an object initialisation. It means there is no choice where to define custom logic. Probably i understand your message wrong. please correct me.

@barasher
Copy link
Owner

I integrated your change "manually", see issue #6

@barasher barasher closed this Aug 25, 2020
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