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

fix "assignment count mismatch: 3 = 2" #40

Merged
merged 1 commit into from Jan 27, 2015
Merged

fix "assignment count mismatch: 3 = 2" #40

merged 1 commit into from Jan 27, 2015

Conversation

nwolff
Copy link
Contributor

@nwolff nwolff commented Jan 27, 2015

This fixes the "Assignment count mismatch: 3 = 2" compilation error introduced in 368fb46

Now the situation is back to what it was before that pull-request: the zip file does not get closed, but at least it compiles.

Would you want me to create a wercker CI configuration so that the code is compiled and tested after each commit to master?

GeertJohan pushed a commit that referenced this pull request Jan 27, 2015
fix "assignment count mismatch: 3 = 2"
@GeertJohan GeertJohan merged commit ca18aeb into GeertJohan:master Jan 27, 2015
@GeertJohan
Copy link
Owner

Thanks for noticing this!
Apparently I made a mistake when testing the change for compilation errors, I think I failed to pull the latest code from that branch...

I think a build check is very welcome. Wercker does about the same as Travic-CI? Should I register at wercker?

@GeertJohan
Copy link
Owner

I would like it if PR's also get built+tested...

@nwolff
Copy link
Contributor Author

nwolff commented Jan 27, 2015

Hi, I created a second PR with the wercker config and the answer to your questions: #41

@abourget
Copy link
Contributor

abourget commented Feb 2, 2015

yeah, sorry folks.. I had applied this patch to https://github.com/daaku/go.zipexe without noticing, thinking it was inside go.rice. Maybe we can vendor in zipexe .. it's a single 135 lines file.

diff --git a/zipexe.go b/zipexe.go
index bfdec56..535e628 100644
--- a/zipexe.go
+++ b/zipexe.go
@@ -12,20 +12,20 @@ import (
 )

 // Opens a zip file by path.
-func Open(path string) (*zip.Reader, error) {
+func Open(path string) (io.Closer, *zip.Reader, error) {
        file, err := os.Open(path)
        if err != nil {
-               return nil, err
+               return nil, nil, err
        }
        finfo, err := file.Stat()
        if err != nil {
-               return nil, err
+               return nil, nil, err
        }
        zr, err := NewReader(file, finfo.Size())
        if err != nil {
-               return nil, err
+               return nil, nil, err
        }
-       return zr, nil
+       return file, zr, nil
 }

 // Open a zip file, specially handling various binaries that may have been

@GeertJohan
Copy link
Owner

Maybe first ask if @daaku is interested in adding a new constructor, e.g. OpenCloser(..)..?

If not, we might add just this constructor to go.rice, it uses the method NewReader which is exported.

@daaku
Copy link

daaku commented Feb 2, 2015

Sure that seems reasonable.

@abourget
Copy link
Contributor

abourget commented Feb 2, 2015

whoops, I created a PR vendoring in zipexe here #42 ... it changes a bit of the semantics, but seems slightly cleaner.

What do you think ?

@daaku
Copy link

daaku commented Feb 2, 2015

That makes Open unrelated to zipexe. Your suggested change above is what I was expecting so it's possible to close the underlying file once we're done with it and the associated *zip.Reader.

@GeertJohan
Copy link
Owner

Yeah, I prefer adding a new constructor in daaku/zipexe instead of vendoring zipexe to this repo.
Ofcourse combining the reader with the closer is cleaner, but that would break the existing API.

@daaku
Copy link

daaku commented Feb 2, 2015

+1 @GeertJohan - add zipexe.OpenCloser and make it like #40 (comment)

@abourget
Copy link
Contributor

abourget commented Feb 2, 2015

Fine, but Open() is still in a broken state.. since leaking open files is obviously a bug.

So I'll write OpenCloser() to fix my immediate needs, and I closed that other PR.

@daaku
Copy link

daaku commented Feb 2, 2015

Yeah Open is broken, though it wasn't terrible since usually it was used to open the running binary itself and needed to survive until the binary exited anyways.

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.

None yet

4 participants