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

Makes Chunk directory configurable #41

Merged
merged 3 commits into from
Dec 20, 2022
Merged

Makes Chunk directory configurable #41

merged 3 commits into from
Dec 20, 2022

Conversation

cuducos
Copy link
Owner

@cuducos cuducos commented Dec 4, 2022

Closes #39

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")
Copy link
Contributor

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

Copy link
Owner Author

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 of lineCount for package variables.

@@ -67,6 +67,7 @@ var (
chunkSize int64
waitBetweenRetries time.Duration
restartDownloads bool
progressDir string
Copy link
Collaborator

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)?

Copy link
Owner Author

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")
Copy link
Collaborator

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).

Copy link
Owner Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Got it 11c608a

cmd/chunk/main.go Show resolved Hide resolved
progress_test.go Show resolved Hide resolved
@cuducos cuducos merged commit b77a565 into main Dec 20, 2022
@cuducos cuducos deleted the chunk-dir-config branch October 24, 2023 16:10
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.

Make Chunk directory configurable
3 participants