Adds get file size method

[vmesel]

Hey, I've seen that you guys are intending to use the Content Size header from the request as a way to get the file size. As I'm learning Go, here is my small contribution to #3.

Am I on the right path, @cuducos?

[cuducos]

Many thanks for the contribution, @vmesel!

On top of the inline comments, I think I have three further questions:

• having the file size might be a good reason to send ch <- DownloadStatus{} imagining the user is building a progress bar, for example; have you considered sending that info through the channel?
• on that topic, should we check all the file sizes before starting the download, so then the user knows this info asap?
• are you planning to write tests (and/or need help with that)?

[vmesel]

Hey @cuducos, thanks for the reply

• having the file size might be a good reason to send ch <- DownloadStatus{} imagining the user is building a progress bar, for example; have you considered sending that info through the channel?

It would be a good idea, I don't know a lot about download internals, but I thought that whenever someone starts a download, we already know the size we are expecting to receive. Is it right?

• on that topic, should we check all the file sizes before starting the download, so then the user knows this info asap?

Probably we should be able to do that and even let the user get just the file size from a link he/she/they use on the main command.

• are you planning to write tests (and/or need help with that)?

Yes, I intend to, but I need some help mocking requests. From my POV, we should mock some requests and send multiple Content-Size headers.

[cuducos]

I thought that whenever someone starts a download, we already know the size we are expecting to receive. Is it right?

Nope. We just know the URL.

Yes, I intend to, but I need some help mocking requests. From my POV, we should mock some requests and send multiple Content-Size headers.

In Go we don't use mocks that much, preferring dependency injection. I think that if we propagate the file size in the channel we return to the user, as ch <- DownloadStatus{…}, we can test that “side-effect” instead. Does that make sense, @danielfireman?

[danielfireman]

Yes, I intend to, but I need some help mocking requests. From my POV, we should mock some requests and send multiple Content-Size headers.

In Go we don't use mocks that much, preferring dependency injection.
I think that if we propagate the file size in the channel we return to the user, as ch <- DownloadStatus{…}, we can test that “side-effect” instead. Does that make sense, @danielfireman?

That sounds good to me, @cuducos. That would also make tests easier, as we could benefit from the tests we already have in place.

[cuducos]

Also… should we retry the HEAD request? I think so…

[vmesel]

Also… should we retry the HEAD request? I think so…

Should it be updated during download ir retried if status code is different than 200?

[cuducos]

Retried in case of failure (e.g. status != 200)

[vmesel]

@cuducos Should this retry method stay inside the getSize function or it should be on the getSize() call?

[cuducos]

I'd say inside, just as the example I linked (high-level function downloadFile is equivalent to getSize, and retry is wrapped inside it).

[danielfireman]

Pretty awesome stuff! Almost there!

[cuducos]

(I just renamed the PR to better reflect what's being suggested, let me knot if I've messed it up)

[vmesel]

Gentlemen, I think we are done with this review. What do you guys think?

[danielfireman]

Thanks for going through the changes! It looks good on my side. Only small comments are below.

[cuducos]

Minor suggestions, but all good. Let me know if, after adding the TODO suggested by @danielfireman, you wanna squash your commits (otherwise I do it on my end).

[vmesel]

Gentlemen, "It's time"! Let's merge it?

cc @cuducos @danielfireman

[cuducos]

Gentlemen, "It's time"! Let's merge it?

Unfortunately, no; the CI is 🔴 for tests and style

If you wanna take a quick look, I suggested some quick wins to tackle both issues while revising your code. I just pushed it to a temporary branch.