Extract downloadWriter from download command

[jdsutherland]

These changes create a downloadWriter type that handles writing solution files and metadata.

[kytrinyx]

This is a really big jump, and I'm not convinced that this is the right abstraction.

A lot of this change has to do with the solution files specifically. Would it make sense to explore something that only encapsulates solution-file related logic first, and see where that goes?

[jdsutherland]

It doesn't seem like there are that many potential ways for this to end up. Can you give more of a sense of the issue you have with it?

I understand splitting this up but I don't really see something much different unfolding.

[kytrinyx]

I feel like it has several jumps that make it hard to reason about the code. It hides information that seems necessary to understand what is going on.

Let me sit down on Sunday and mess around with it for a while so I can get a sense of what the options are.

[jdsutherland]

I see. To me it looks pretty similar to the existing runDownload loop with the difference being that separate responsibilities have been extracted and the long reaching variables are done away with.

I started by pulling out the loop that writes solution files. Having done that, it seems like there are a few distinguishable responsibilities: requestFile and sanitizeLegacyFilepath. I also extracted destination as it is used a few places and seems worth following Demeter.

To my eye, this seems like a reasonable split of responsibilities. I tried starting over again but it didn't change anything for me; maybe my eyes are no good because I've looked at it too long. I'd be interested to see what you would suggest here.

[kytrinyx]

Cool, yeah you might be right that this is the best split of responsibilities! I'm now very curious to see where this goes :-) I have time scheduled tomorrow to play with it.

[nywilken]

@jdsutherland thanks for pushing this command forward and the work to simplify it.

I see where you are going with this but I don't think breaking the logic out into a writer type gets us closer to reducing the load of the download command. It feels like we are just moving the complexity deeper down, which tends to hide the intent of the code. I left a few comments around the changes to see if I can help provide guidance, but my first question is "What are we aiming for with the refactor?"

[nywilken]

I think this method might be going beyond the scope of the download type. Which If I understand correctly is a custom type used for parsing flags and setting up the download command. Having actual exercise metadata and solutionFile data ⬇️ tied to it doesn't seem correct.

[nywilken]

I could see this being its own function not tied to download. Maybe this is something that should live on the Exercism Client API with a single argument being the parsedURL.string() or in @kytrinyx's Solution File refactor sf.url().

[kytrinyx]

I like the idea of having a helper on the client API with a single URL argument. That feels like a perfectly reasonable (and cohesive!) bit of behavior for the CLI.

[nywilken]

I think we could get a way with just initializing a literal downloadWriter in runDownload and dropping the constructor seeing as it is not doing anything special. With that being said, is there a reason for returning a pointer to a downloadWriter?

[jdsutherland]

@nywilken thanks for your review. I would address your comments but I was under the impression that there isn't interest in pursuing this line based on @kytrinyx response. I was expecting to close this.

[nywilken]

@jdsutherland I think there was some good discovery here. I’ll go ahead and close this PR and hope that we can use the discussion as a point of reference for future PRs. Feel free to open a new PR or issue(s) for the items discussed.