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

(COMMON/FILE) Add Zip create/extract support #411

Merged
merged 11 commits into from Jan 12, 2020
Merged

Conversation

@xtda
Copy link
Member

xtda commented Jan 10, 2020

PR Description

This was originally included as part of #383 but with the addition of extracting support thought it needed to be in its own PR

Basic support for creating and extracting Zip based archives

Unzip(src, dest string) (fileList []string, err error) 
Zip(src, dest string) error 

Zip supports either single file or will walk a folder path if src is a folder

Type of change

  • New feature (non-breaking change which adds functionality)

How has this been tested

Please describe the tests that you ran to verify your changes. Provide instructions so we can reproduce. Please also list any relevant details for your test configuration and
also consider improving test coverage whilst working on a certain feature or package.

  • go test ./... -race
  • golangci-lint run

Checklist

  • My code follows the style guidelines of this project
  • I have performed a self-review of my own code
  • I have commented my code, particularly in hard-to-understand areas
  • I have made corresponding changes to the documentation and regenerated documentation via the documentation tool
  • My changes generate no new warnings
  • I have added tests that prove my fix is effective or that my feature works
  • New and existing unit tests pass locally and on Travis with my changes
  • Any dependent changes have been merged and published in downstream modules
@xtda xtda self-assigned this Jan 10, 2020
@codecov

This comment has been minimized.

Copy link

codecov bot commented Jan 10, 2020

Codecov Report

Merging #411 into master will decrease coverage by 0.28%.
The diff coverage is 55.1%.

Impacted file tree graph

@@            Coverage Diff             @@
##           master     #411      +/-   ##
==========================================
- Coverage   41.26%   40.98%   -0.29%     
==========================================
  Files         161      162       +1     
  Lines       38474    38572      +98     
==========================================
- Hits        15875    15807      -68     
- Misses      21543    21637      +94     
- Partials     1056     1128      +72
Impacted Files Coverage Δ
common/file/archive/zip.go 55.1% <55.1%> (ø)
exchanges/gemini/gemini.go 81.3% <0%> (-8.42%) ⬇️
exchanges/localbitcoins/localbitcoins.go 48.92% <0%> (-4.29%) ⬇️
exchanges/binance/binance.go 77.2% <0%> (-3.99%) ⬇️
exchanges/poloniex/poloniex_wrapper.go 47.65% <0%> (-3.13%) ⬇️
exchanges/poloniex/poloniex.go 38.67% <0%> (-2.78%) ⬇️
exchanges/binance/binance_wrapper.go 54.52% <0%> (-2.45%) ⬇️
exchanges/gemini/gemini_wrapper.go 44.61% <0%> (-2.4%) ⬇️
exchanges/bitstamp/bitstamp.go 80.98% <0%> (-2.35%) ⬇️
exchanges/localbitcoins/localbitcoins_wrapper.go 41.61% <0%> (-2.32%) ⬇️
... and 4 more

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update 44ac358...51fa5a7. Read the comment docs.

Copy link
Collaborator

gloriousCode left a comment

Looks good. Tests pass. Just some minor requests

common/file/archive/zip_test.go Outdated Show resolved Hide resolved
common/file/archive/zip_test.go Outdated Show resolved Hide resolved
common/file/archive/zip.go Show resolved Hide resolved
common/file/archive/zip_test.go Outdated Show resolved Hide resolved
common/file/archive/zip.go Outdated Show resolved Hide resolved
xtda added 2 commits Jan 10, 2020
Copy link
Collaborator

thrasher- left a comment

Nice PR! Just 2 minor things

common/file/archive/zip.go Show resolved Hide resolved
})
}

func addFilesToZip(z *zip.Writer, src, dest string) error {

This comment has been minimized.

Copy link
@thrasher-

thrasher- Jan 10, 2020

Collaborator

This appears to be unused ATM

This comment has been minimized.

Copy link
@xtda

xtda Jan 10, 2020

Author Member

:D latest push completes this if you could give it another try please!

xtda added 2 commits Jan 10, 2020
… coverage
var dir bool
if i.IsDir() {
dir = true
}
Comment on lines 124 to 127

This comment has been minimized.

Copy link
@gloriousCode

gloriousCode Jan 10, 2020

Collaborator

You could simplify this with dir := i.IsDir()
Or you could do err = addFilesToZip(z, src, i.IsDir())

Copy link
Collaborator

gloriousCode left a comment

Thanks for making those changes! Last two things, and I'm all good

common/file/archive/zip.go Outdated Show resolved Hide resolved
return err
}

h, err := zip.FileInfoHeader(i)

This comment has been minimized.

Copy link
@gloriousCode

gloriousCode Jan 10, 2020

Collaborator

If you declare var h *FileHeader, you get past the err redeclaration

common/file/archive/zip.go Outdated Show resolved Hide resolved
common/file/archive/zip.go Outdated Show resolved Hide resolved
Copy link
Collaborator

gloriousCode left a comment

Thanks for making those changes 🚿 💧 🏠 💥

Copy link
Collaborator

thrasher- left a comment

2 minor nitterinos

z.Close()
errRemove := os.Remove(dest)
if errRemove != nil {
log.Debugf(log.Global, "failed to remove archive, manual deletion required: %v", errRemove)

This comment has been minimized.

Copy link
@thrasher-

thrasher- Jan 10, 2020

Collaborator

Please change to log.Errorf and failed to Failed

}
_, err = io.Copy(w, f)
if err != nil {
log.Debugf(log.Global, "Failed to Copy data: %v", err)

This comment has been minimized.

Copy link
@thrasher-

thrasher- Jan 10, 2020

Collaborator

Please change to log.Errorf

common/file/archive/zip.go Show resolved Hide resolved
Copy link
Collaborator

thrasher- left a comment

Thanks for making those changes! tACK

@MadCozBadd

This comment has been minimized.

Copy link
Collaborator

MadCozBadd commented Jan 12, 2020

tACK!

Copy link
Collaborator

shazbert left a comment

tACK - Fine work!

Copy link
Collaborator

gloriousCode left a comment

tACK

@thrasher- thrasher- merged commit 12159e3 into thrasher-corp:master Jan 12, 2020
1 of 3 checks passed
1 of 3 checks passed
Review Action Errored whilst reviewing.
Details
continuous-integration/appveyor/pr AppVeyor build failed
Details
Travis CI - Pull Request Build Passed
Details
@xtda xtda mentioned this pull request Jan 13, 2020
10 of 10 tasks complete
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
5 participants
You can’t perform that action at this time.