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

add receive feature #74

Merged
merged 3 commits into from Aug 16, 2018

Conversation

2 participants
@brualan
Contributor

brualan commented Aug 8, 2018

More info: #58

@claudiodangelis

If you agree, please address issues below. After that I have more comments about how the code is structured, but some issues need to be addressed first. Thanks!

main.go Outdated
@@ -17,37 +17,45 @@ var forceFlag = flag.Bool("force", false, "ignore saved configuration")
var debugFlag = flag.Bool("debug", false, "increase verbosity")
var quietFlag = flag.Bool("quiet", false, "ignores non critical output")
var portFlag = flag.Int("port", 0, "port to bind the server to")
var receiveMode = flag.Bool("recieve", false, "allows to receive files")

This comment has been minimized.

@claudiodangelis

claudiodangelis Aug 8, 2018

Owner
  1. please replace with receiveFlag, for consistency
  2. please fix typo recieve -> receive
  3. please reword the description to "receives files"
}
serveFilesHTTP(generatedAddress, route, content, wg, stopSignal)
defer func() {

This comment has been minimized.

@claudiodangelis

claudiodangelis Aug 8, 2018

Owner

I'm not 100% sure why this function call should be deferred. Any hint?

This comment has been minimized.

@brualan

brualan Aug 8, 2018

Contributor

Previously, if the program argument was a directory, the directory was compressed. In order to avoid unnecessary computation, the getContent() call is performed only when the program is supposed to serve files. But at the same time, the content should be deleted under certain conditions and not earlier than the server will finish its work. Because of this, the deletion must be done last. For such purposes, defer is used.

}
}
// create output file if error was handled succesfully
out, _ = os.Create(filepath.Join(dirToStore, fileHeader.Filename))

This comment has been minimized.

@claudiodangelis

claudiodangelis Aug 8, 2018

Owner

Please check error here

stop <- true // send signal to server to shutdown
}
// create output file if error was handled succesfully
out, _ = os.Create(filepath.Join(dirToStore, fileHeader.Filename))

This comment has been minimized.

@claudiodangelis

claudiodangelis Aug 8, 2018

Owner

Check error here as well

This comment has been minimized.

@brualan

brualan Aug 8, 2018

Contributor

All errors handled above. At this point there will be no error

// try to create output file
out, err := os.Create(filepath.Join(dirToStore, fileHeader.Filename))
defer out.Close()

This comment has been minimized.

@claudiodangelis

claudiodangelis Aug 8, 2018

Owner

Move defer out.Close() after checking the error

// try to create output file
out, err := os.Create(filepath.Join(dirToStore, fileHeader.Filename))
defer out.Close()
if err != nil {

This comment has been minimized.

@claudiodangelis

claudiodangelis Aug 8, 2018

Owner

For sake of readability, please complement the behavior here: if err == nil, then return/abort/cleanup or do something else.

defer out.Close()
if err != nil {
if os.IsNotExist(err) {
err := os.MkdirAll(dirToStore, os.ModePerm)

This comment has been minimized.

@claudiodangelis

claudiodangelis Aug 8, 2018

Owner

This can be refactored:

if err := os.MkdirAll(.....); err != nil {
    // fmt.fprintf... unable to create specified dir....
}

Also, double check spelling please.

out, _ = os.Create(filepath.Join(dirToStore, fileHeader.Filename))
// write the content from POSTed file to the out
_, err = io.Copy(out, file)

This comment has been minimized.

@claudiodangelis

claudiodangelis Aug 8, 2018

Owner

This can be simplified:

if _, err := io.Copy(out, file); err != nil {
    // unable to write file to disk....
}
stop <- true // send signal to server to shutdown
}
fmt.Fprintf(w, "File uploaded successfully: %s\n", fileHeader.Filename) //ouput to server

This comment has been minimized.

@claudiodangelis

claudiodangelis Aug 8, 2018

Owner

This code is reached even if err != nil, resulting in confusing message

fix receive flag
More info:
rename variable name "receiveMode" to "receiveFlag"
fix typo in flag's name
change flag's description
@claudiodangelis

This comment has been minimized.

Owner

claudiodangelis commented Aug 8, 2018

Sorry I accidentally clicked on "approve changes" and I cannot undo the action for some reason :-/

@brualan

This comment has been minimized.

Contributor

brualan commented Aug 8, 2018

@claudiodangelis What should the program do if the user uploads a file with a name that is already taken. Maybe change the name of a new file like this photo.jpg -> photo(1).jpg and notify a user about that. Or should be this done some other way?

@claudiodangelis

This comment has been minimized.

Owner

claudiodangelis commented Aug 8, 2018

@claudiodangelis

This comment has been minimized.

Owner

claudiodangelis commented Aug 8, 2018

@brualan

This comment has been minimized.

Contributor

brualan commented Aug 8, 2018

@claudiodangelis I see it like that: if the user uploads a file with a name that is already taken filename changes to original_file_name + (number) and when transfer complete we print File [original_file_name + (number)] was successfully upload

@claudiodangelis

This comment has been minimized.

Owner

claudiodangelis commented Aug 8, 2018

I see it similarly: we define a "filename" variable set to the uploading file. If the uploading filename already exists, we change the variable name by adding a suffix. At the end of the process, we print the filename variable which may or may not have been edited since the initial definition.

@claudiodangelis

This comment has been minimized.

Owner

claudiodangelis commented Aug 8, 2018

I'm leaving my desk now, please expect delayed responses. 🌚

@brualan

This comment has been minimized.

Contributor

brualan commented Aug 10, 2018

@claudiodangelis Have you seen last changes?

@claudiodangelis

This comment has been minimized.

Owner

claudiodangelis commented Aug 13, 2018

Yep, it works great, many thanks for this. I think there is still room for improvement though, please allow me some time to do a second review.

@claudiodangelis

This comment has been minimized.

Owner

claudiodangelis commented Aug 13, 2018

For example, what about renaming the flag to -receive-to?

As of now what happens is extremely confusing because there is no indication to the user of what the first argument will be used for if the -receive flag is used, and if no arg is passed the program quits with a generic "at least one argument is required".

@brualan

This comment has been minimized.

Contributor

brualan commented Aug 13, 2018

@claudiodangelis In the majority of programs, the flags reflect not the specific behavior of the program, but the concept of what it will do (extract, help, recursive, and so on). Perhaps for consistency it's worth to leave just receive?

@claudiodangelis

This comment has been minimized.

Owner

claudiodangelis commented Aug 13, 2018

Yes, completely agree with you, receive-to does not sound good at all. Let's stick to receive, and add an optional -dir (or -out, or please suggest). If -dir/-out is not passed, transfered file is stored into the current working directory, otherwise will use -dir/-out.

Additional args should be ignored, for example in the case of:

qr-filetransfer -receive /path/to/something

the file should be transfered to the current working directory, not to /path/to/something.

What do you think?

@brualan

This comment has been minimized.

Contributor

brualan commented Aug 13, 2018

I do not really like the idea of defining a directory as a separate flag. It is better to leave the current version, when the destination directory is specified after the receive flag. And in the case when the user does not specify a directory, displays the message At least one argument is required. In case you want to save the file in the working directory, you can simply call the program like this qr-filetransfer -receive .

@claudiodangelis

This comment has been minimized.

Owner

claudiodangelis commented Aug 13, 2018

@brualan

This comment has been minimized.

Contributor

brualan commented Aug 13, 2018

Yes, both relative and absolute. Tested only on Linux, but I think there will not be problems on other operating systems, because only standard library functions were used and none of "system-specific hacks"

@claudiodangelis

This comment has been minimized.

Owner

claudiodangelis commented Aug 14, 2018

Meanwhile the project got a logo (arasatasaygin/openlogos#4), I'm awaiting logo's assets so we can include it in the upload page.

@claudiodangelis

This comment has been minimized.

Owner

claudiodangelis commented Aug 14, 2018

I will improve the html tomorrow, include the logo and merge to master 💪

@claudiodangelis claudiodangelis merged commit a872254 into claudiodangelis:master Aug 16, 2018

@claudiodangelis

This comment has been minimized.

Owner

claudiodangelis commented Aug 16, 2018

Merged, thank you @brualan!

@brualan

This comment has been minimized.

Contributor

brualan commented Aug 16, 2018

@claudiodangelis Awesome! Can you please update README, to let people know how to use this feature?

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment