-
-
Notifications
You must be signed in to change notification settings - Fork 3
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
Makes Chunk directory configurable #41
Conversation
progress.go
Outdated
if err != nil { | ||
return "", fmt.Errorf("could not get current user: %w", err) | ||
func getChunkDirectory(custom string) (string, error) { | ||
d := os.Getenv("CHUNK_DIR") |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Shouldn't this variable be named dir
instead? As we use the dir
variables on other functions and it represents the same meaning and directory
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Given the small scope of the function (15 lines), I'm OK with d
— but offer no resistance to change it either lol.
To cite @biancarosa:
Their scope is tiny, the code block that its used is tiny. You can see right where it ends. You don’t have to scroll down too much.
On a more general rule, your variables names should grow according to its scope. Don’t go all crazy and use
c
instead oflineCount
for package variables.
@@ -67,6 +67,7 @@ var ( | |||
chunkSize int64 | |||
waitBetweenRetries time.Duration | |||
restartDownloads bool | |||
progressDir string |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Don't you need to set progressDir
at Downloader (i.e., line 50)?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Ooops. Sure thing! Done in 3c17cdf
progress.go
Outdated
if err != nil { | ||
return "", fmt.Errorf("could not get current user: %w", err) | ||
func getChunkDirectory(custom string) (string, error) { | ||
d := os.Getenv("CHUNK_DIR") |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Please move this to after check custom
(i.e., line 26).
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Got it 11c608a
Closes #39