Uh oh!
There was an error while loading. Please reload this page.
Add workspace import_dir command to the CLI - #418
Conversation
shreyas-goenka
commented
Jun 1, 2023
outputs: |
Uh oh!
There was an error while loading. Please reload this page.
Uh oh!
There was an error while loading. Please reload this page.
Uh oh!
There was an error while loading. Please reload this page.
Uh oh!
There was an error while loading. Please reload this page.
| // Initialize error wait group, and spawn the progress event emitter inside | ||
| // the error wait group | ||
| group, ctx := errgroup.WithContext(ctx) |
There was a problem hiding this comment.
What's the behavior of the returned context?
There was a problem hiding this comment.
The returned context is cancelled if there's an error in any of the subroutines. see: https://pkg.go.dev/golang.org/x/sync/errgroup#WithContext
Uh oh!
There was an error while loading. Please reload this page.
Uh oh!
There was an error while loading. Please reload this page.
| if err != nil { | ||
| return err | ||
| } | ||
| _, err = w.Write([]byte("\n")) |
There was a problem hiding this comment.
Can you split this into a separate PR? Might affect other functionality.
Uh oh!
There was an error while loading. Please reload this page.
| return "", fmt.Errorf("could not find remote path for %s", localPath) | ||
| } | ||
| return s.repoFiles.RemotePath(relativePath) | ||
| } |
There was a problem hiding this comment.
Can you add a comment here why this is a public function? Also this needs a mutex because the snapshot code is accessing it from a separate goroutine.
There was a problem hiding this comment.
A better approach is to include the remote name in the events, then you don't need this function or the mutex.
There was a problem hiding this comment.
Yeah, having this in the events is then you have to use reflection to read the remote name values, which is OK maybe
There was a problem hiding this comment.
The type switch is there already right?
There was a problem hiding this comment.
Ah right, using type assertions should be enough here
shreyas-goenka
commented
Jun 6, 2023
Closing because we decided to move forward with just a simple walk dir |
TODO: add integration tests for functionality and progress events