Skip to content
This repository has been archived by the owner on Feb 16, 2024. It is now read-only.

Delete the zip files. (#347) #358

Merged
merged 10 commits into from Mar 29, 2022
Merged

Delete the zip files. (#347) #358

merged 10 commits into from Mar 29, 2022

Conversation

CarsonSlovoka
Copy link
Contributor

@CarsonSlovoka CarsonSlovoka commented Mar 22, 2022

Fix for issue #347


I have written a test, run under windows can pass the test.

test/file_test.go Outdated Show resolved Hide resolved
@asticode
Copy link
Owner

I'm not fully satisfied with the way you've implemented this feature. Here's how I would do things:

  1. change the mover interface to func(ctx context.Context, p Paths) (func() error, error) so that it returns a func
  2. for both default movers, return a func() error that will remove the zip file. For instance, for astilectron:
func() (err error) {
  if err = os.RemoveAll(p.AstilectronDownloadDst()); err != nil {
    err = fmt.Errorf("removing %s failed: %w", p.AstilectronDownloadDst(), err)
    return
  }
}
  1. then in provisionPackage, replace the move logic with something like:
// Move
var closeFunc func() error
if err = m(ctx, paths); err != nil {
	return fmt.Errorf("moving %s failed: %w", name, err)
}

// Make sure to close
defer func() {
  if err := closeFunc(); err != nil {
    // Only log the error
    p.l.Error(fmt.Errorf("closing failed: %w", err))
    return
  }
}()

@coveralls
Copy link

coveralls commented Mar 25, 2022

Coverage Status

Coverage decreased (-0.5%) to 78.44% when pulling 6daec70 on CarsonSlovoka:is347 into 18cf1d5 on asticode:master.

provisioner.go Outdated Show resolved Hide resolved
provisioner.go Outdated Show resolved Hide resolved
provisioner.go Show resolved Hide resolved
provisioner.go Outdated Show resolved Hide resolved
provisioner.go Outdated Show resolved Hide resolved
provisioner_test.go Outdated Show resolved Hide resolved
provisioner_test.go Outdated Show resolved Hide resolved
provisioner_test.go Outdated Show resolved Hide resolved
provisioner_test.go Outdated Show resolved Hide resolved
provisioner_test.go Outdated Show resolved Hide resolved
@CarsonSlovoka
Copy link
Contributor Author

Thank you for taking the time to help me do the code review. If you feel that teaching me is too slow, you can directly change it doesn't matter. I will then observe how you are dealing with it.

astilectron.go Outdated Show resolved Hide resolved
astilectron.go Outdated Show resolved Hide resolved
pkg/README.md Outdated Show resolved Hide resolved
pkg/testing/log.go Outdated Show resolved Hide resolved
provisioner_test.go Outdated Show resolved Hide resolved
provisioner_test.go Show resolved Hide resolved
provisioner.go Outdated Show resolved Hide resolved
provisioner.go Outdated Show resolved Hide resolved
provisioner.go Outdated Show resolved Hide resolved
provisioner.go Outdated Show resolved Hide resolved
provisioner_test.go Outdated Show resolved Hide resolved
provisioner_test.go Outdated Show resolved Hide resolved
provisioner_test.go Outdated Show resolved Hide resolved
@asticode asticode merged commit 1a9ca6a into asticode:master Mar 29, 2022
@asticode
Copy link
Owner

Thanks for this PR 👍

Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

3 participants