Refactor download cmd - compose & extract shared behavior for new doctor cmd #756

jdsutherland · 2018-10-26T13:09:20Z

These changes are motivated by #744, in which the new doctor command needs to download missing metadata for exercises in the workspace. cmd/download contained the logic for this but to be usable it needed refactoring; due to the overlapping need for downloading metadata, it makes sense to extract it outside of an individual command and split the download logic into separate functions.

jdsutherland · 2018-11-03T10:19:32Z

Questionable points:

The filename download_helper doesn't feel quite right.
Current test coverage is from download_test. Since there will be shared behavior among download and doctor that should be tested (Update metadata file when downloading a solution #697 is an example), we might want to refactor some of the existing tests into unit tests for these changes and adapt download_test to verify the right calls are made (mocks).

kytrinyx

This looks like a really nice improvement ✨

I have a few suggestions/questions. Would love to hear your thoughts!

kytrinyx · 2018-11-05T21:12:05Z

cmd/download.go

@@ -79,191 +69,31 @@ func runDownload(cfg config.Config, flags *pflag.FlagSet, args []string) error {
 		return err
 	}

-	param := "latest"


It feels so good to get all this logic out of here! 😍

kytrinyx · 2018-11-05T21:14:10Z

cmd/download_helper.go

@@ -0,0 +1,244 @@
+package cmd


I think you're right that download_helper.go feels a bit odd here. I wonder if we could just stick all the shared logic into cmd/cmd.go so that all files in the rest of this package are either command files or test files.

The other option is to stick this logic somewhere else (different package) but that feels like a much bigger hammer, and I'd like to wait before we go down that route.

cmd/download_helper.go

kytrinyx

A few comments/suggestions. I really love where this is going!

kytrinyx · 2018-11-07T20:31:46Z

cmd/cmd.go

+	payload *downloadPayload
+}
+
+func newDownload(ctx *downloadContext) error {


I'm not sure newDownload is the right name here. Generally if I see a function newDownload I would expect it to return a Download value or a pointer to a download value. In this case it performs a download, right?

kytrinyx · 2018-11-07T20:32:37Z

cmd/cmd.go

+	}
+
+	query := req.URL.Query()
+	ctx.buildQuery(query)


Should the buildQuery initialize the query itself as well? Something like:

query := ctx.buildQuery()

I don't see a better way of doing it. Initializing requires a reference to the *http.Request, right? I don't see how we can avoid having to pass that in.

Edit: Actually, there is a better way of doing it (though it still requires passing in a reference to the request).

kytrinyx · 2018-11-07T20:33:08Z

cmd/cmd.go

+	return nil
+}
+
+func (d *downloadContext) writeMetadata() error {


Sometimes we've called this d other times we've called it ctx. I think it would be worth being consistent.

kytrinyx · 2018-11-07T20:34:03Z

cmd/cmd.go

+	return nil
+}
+
+func (d *downloadContext) getExercise() workspace.Exercise {


Style-wise in the CLI we typically don't use get prefixes for getters, we'll usually name the method after what it returns (so exercise()).

kytrinyx · 2018-11-07T20:34:22Z

cmd/cmd.go

+	}
+}
+
+func (d *downloadContext) getMetadata() workspace.ExerciseMetadata {


Ditto here, this should probably be metadata().

jdsutherland · 2018-11-08T02:58:08Z

Thanks for the feedback.

Do we want to make any changes to the tests? I think we might be forced into making a move since there will be shared behavior behavior on download and doctor (#697 - unless we think it's not worth explicitly testing).

I think it's also worth noting (though might not directly affect this PR) that there are a few other methods that live in download_test and submit_test that doctor_test probably wants to share: fakeDownloadServer and fakeSubmitServer. If these turn out to be shareable (it seems likely), should they be extracted to cmd_test?

kytrinyx · 2018-11-09T02:07:01Z

should they be extracted to cmd_test

Yeah, I think that would be a good move. It means that if we don't find the helper method in the test file we're in, we know to look for it there.

Based on exercism#794, we've decide to error out and display API error messages. This overlaps with cmd/submit, so this change extracts it for sharing.

The body is small enough that it doesn't seem worth the cost of extracting.

jdsutherland · 2019-02-24T05:14:15Z

This has been sitting for awhile and I don't plan on making further changes. This message will serve as a status of present concerns:

I don't think cmd/cmd is a good home for the download related stuff. Currently, download specific stuff takes up ~80% of the file where the remainder contains abstract shared utils. It seems that download is clearly a separate concept when compared to the rest of the file and therefore belongs elsewhere.

If we move it to a new file in the cmd package, it's hard to think of a good name (we already have cmd/download). It seems appropriate to pull this out into a separate package. This seems preferable to an awkwardly named file in the cmd package when this isn't really a command but rather a service consumed by commands. On the other hand, I see that creating a new package for this one specific thing (and especially if it isn't likely to grow) isn't ideal, but I don't see a more sensible solution.

Unrelated: I had previously expressed concerns about testing but I don't think it's worth worrying about.

kytrinyx · 2019-02-26T21:34:36Z

Thanks @jdsutherland. I'm going to close this, but it has been a really useful discussion. The download command is a bit of a monster and I honestly don't quite know where to go with it.

jdsutherland · 2019-02-27T01:51:00Z

I'm a bit surprised by the jump to closing this without discussion and I don't really have a sense as to why. In the interest of feedback, it'd be useful to understand the thought process here - was this just so far off the mark that it wasn't worth iterating on?

kytrinyx · 2019-02-27T04:55:18Z

@jdsutherland You said you weren't going to work on it anymore. Did I misunderstand?

jdsutherland · 2019-02-27T05:01:48Z

@kytrinyx I see how what I said could be misunderstood.

The previous message was just an attempt to give an up to date status - I had posted several comments in the past that I now deem outdated based on iterating on this so I was trying to say: this seems stable and I don't plan on changing this until I get further feedback.

Sorry for the confusion.

kytrinyx · 2019-02-27T05:11:58Z

Oh my goodness I'm so sorry. I totally see how that can be read both ways, now that I know what you intended!

So. Since I thought you had given up on this one, I basically took the direction that I liked about it, and extracted a bunch of little bits and pieces into separate PRs (I used the co-authored-by tag to give you credit) and merged them into master. Then I opened this as a suggested next step: #820

The main idea is based on what you did here, but it has fewer abstractions.

Would you pull that down and see if it would solve the "extract so we can re-use" problem for you?
Would you take a look at it in terms of readability and see if you think we should take it in a different direction?

jdsutherland · 2019-02-27T05:47:18Z

No worries.

I'm not sure if it's best to put things here or in #820, but I'll start here because this PR is relevant in reference to what I'm saying (I'll move it if we decide otherwise).

I think this PR (#756) may be overly abstract or complex than is needed but I think we're going to want more pulled out than is currently in #820.

I envision the part of doctor that migrates legacy metadata will look like this:

exercises, err := ws.PotentialExercises()
if err != nil {
	return err
}

for _, exercise := range exercises {
	if _, err := exercise.MigrateLegacyMetadataFile(); err != nil {
		return err
	}
	if ok, _ := exercise.HasMetadata(); !ok {
		download, err := newDownloadFromExercise(exercise, cfg.UserViperConfig)
		if err != nil {
			return err
		}

		writer, err := newDownloadWriter(download)
		if err != nil {
			return err
		}

		if err := writer.writeMetadata(); err != nil {
			return err
		}
	}
}

If you think about implementing this from #820, there's a lot of potential duplication among shared behavior. doctor will have much more in common with download than is currently pulled out. For example:

validation
handling the download response
writing metadata (including destination)

It doesn't seem that really any of the behavior is going to differ between them so it seems appropriate to abstract it out and encapsulate it.

I think there's good potential future reasons for pulling it out as well (#718 is one example).

kytrinyx · 2019-02-27T14:50:23Z

Yepp, that makes perfect sense to me.

I think I'd prefer to do the extractions in multiple PRs, though, rather than in one big one, since it is so much harder to review a big PR (which means I end up putting it off much longer).

Shall we see if we can get #820 into a reasonable state for what it's currently doing, and then tackle the next extractions in a series of smaller PRs, discussing how far to take things each time?

jdsutherland · 2019-02-28T01:48:28Z

Yeah, I started this not really knowing where it would go and it became quite large; breaking it into smaller PRs makes sense.

I'm not sure how I feel about moving forward with #820. I've put a good amount of work into this PR and it seems to facilitate what's needed for the doctor command. I'm happy to adjust things based on review but throwing most of it out without feedback and starting over from another point doesn't make much sense to me. If there's a good reason for doing so, I'm interested to understand why but I currently don't see it.

kytrinyx · 2019-02-28T04:52:18Z

Ok, sure. Let's move forward with this one. I'll close #820.

However, would you be willing to break it up into smaller pieces, one at a time? It's really challenging to review a change that is this big. If you have an idea of where you're going, could we take it in smaller steps?

jdsutherland · 2019-02-28T05:06:20Z

Sure. Given how many garbage commits this has (and it has conflicts that need resolved anyway), it might make more sense to start from master.

Since that I can't unsee this, it's probably going to be hard to me to gauge what comprehensible steps look like. Should we proceed by me guessing and go from there or should we try to clear certain things up first?

For example, is it possible to look at it now and say that there are concerns about the main abstractions?

kytrinyx · 2019-02-28T05:16:55Z

Since that I can't unsee this, it's probably going to be hard to me to gauge what comprehensible steps look like.

Yeah, I know the feeling :)

it might make more sense to start from master.

I think that makes sense.

My main concern is that there are so many abstractions I think that having fewer abstractions, at least to start, would make more sense. Let me go through the PR and see if I can suggest some first steps.

kytrinyx · 2019-02-28T05:27:38Z

Ok, what do you think about starting off with a very simple download type similar to what you have, and creating it from flags? I'd like to avoid creating a separate params type and validator type, and have that behavior on the download type.

In this first step, I'd like to avoid thinking about what the doctor command needs just yet, and just work towards a minimal change that is better than what we currently have.

kytrinyx · 2019-02-28T05:30:22Z

Looking at master and at your code again, I think that we'll end up pulling all the flag stuff out of addQueryToDownloadRequest and downloadIdentifier straight into your abstraction, and do what you did with the params and validation: pull stuff out of the flags so you can deal with the errors up front, and then validate the logic without having to deal with the errors in the same flow.

jdsutherland · 2019-02-28T06:56:37Z

#821 is an attempt at following what you've said. My concern is that this can seem bad in the context of intermediate steps but in the finished state, it makes more sense.

It's possible that the types I have aren't necessary but I think that download may end up being large and having multiple responsibilities whereas I was viewing it as a simple value object that just holds data.

jdsutherland · 2019-03-07T23:59:39Z

This is going in a different direction so it seems fit to close this.

jdsutherland force-pushed the extract-download-logic branch from 46761cc to cfcf8e9 Compare October 26, 2018 13:31

jdsutherland changed the title ~~Extract download logic~~ Refactor extract download logic Oct 26, 2018

jdsutherland force-pushed the extract-download-logic branch 4 times, most recently from 80c9ff4 to 20851be Compare October 27, 2018 03:08

jdsutherland mentioned this pull request Oct 28, 2018

[WIP] Add doctor cmd - migrate legacy exercises #744

Closed

jdsutherland force-pushed the extract-download-logic branch from 1444385 to 1b83874 Compare November 3, 2018 09:59

jdsutherland force-pushed the extract-download-logic branch from 01dfa38 to d409dae Compare November 4, 2018 00:40

kytrinyx reviewed Nov 5, 2018

View reviewed changes

jdsutherland force-pushed the extract-download-logic branch 4 times, most recently from 6455cc8 to deded6c Compare November 6, 2018 08:47

kytrinyx reviewed Nov 7, 2018

View reviewed changes

jdsutherland force-pushed the extract-download-logic branch from abbd257 to 75218ba Compare November 8, 2018 00:23

jdsutherland force-pushed the extract-download-logic branch 11 times, most recently from f99d7b5 to 29cf184 Compare December 12, 2018 03:33

jdsutherland force-pushed the extract-download-logic branch 3 times, most recently from c934fc6 to 738f5f5 Compare February 22, 2019 06:14

Download non-200 response no longer swallowed

f87b49c

Based on exercism#794, we've decide to error out and display API error messages. This overlaps with cmd/submit, so this change extracts it for sharing.

jdsutherland force-pushed the extract-download-logic branch from 738f5f5 to f87b49c Compare February 22, 2019 06:17

Merge writeSolutionFile methods

e3d0b2b

The body is small enough that it doesn't seem worth the cost of extracting.

kytrinyx closed this Feb 26, 2019

kytrinyx reopened this Feb 28, 2019

jdsutherland closed this Mar 7, 2019

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Refactor download cmd - compose & extract shared behavior for new doctor cmd #756

Refactor download cmd - compose & extract shared behavior for new doctor cmd #756

jdsutherland commented Oct 26, 2018 •

edited

Loading

jdsutherland commented Nov 3, 2018 •

edited

Loading

kytrinyx left a comment

kytrinyx Nov 5, 2018

kytrinyx Nov 5, 2018

kytrinyx left a comment

kytrinyx Nov 7, 2018

kytrinyx Nov 7, 2018

jdsutherland Nov 8, 2018 •

edited

Loading

kytrinyx Nov 7, 2018

kytrinyx Nov 7, 2018

kytrinyx Nov 7, 2018

jdsutherland commented Nov 8, 2018 •

edited

Loading

kytrinyx commented Nov 9, 2018

jdsutherland commented Feb 24, 2019 •

edited

Loading

kytrinyx commented Feb 26, 2019

jdsutherland commented Feb 27, 2019 •

edited

Loading

kytrinyx commented Feb 27, 2019

jdsutherland commented Feb 27, 2019

kytrinyx commented Feb 27, 2019

jdsutherland commented Feb 27, 2019

kytrinyx commented Feb 27, 2019

jdsutherland commented Feb 28, 2019 •

edited

Loading

kytrinyx commented Feb 28, 2019

jdsutherland commented Feb 28, 2019 •

edited

Loading

kytrinyx commented Feb 28, 2019

kytrinyx commented Feb 28, 2019

kytrinyx commented Feb 28, 2019

jdsutherland commented Feb 28, 2019

jdsutherland commented Mar 7, 2019

Refactor download cmd - compose & extract shared behavior for new doctor cmd #756

Refactor download cmd - compose & extract shared behavior for new doctor cmd #756

Conversation

jdsutherland commented Oct 26, 2018 • edited Loading

jdsutherland commented Nov 3, 2018 • edited Loading

kytrinyx left a comment

Choose a reason for hiding this comment

kytrinyx Nov 5, 2018

Choose a reason for hiding this comment

kytrinyx Nov 5, 2018

Choose a reason for hiding this comment

kytrinyx left a comment

Choose a reason for hiding this comment

kytrinyx Nov 7, 2018

Choose a reason for hiding this comment

kytrinyx Nov 7, 2018

Choose a reason for hiding this comment

jdsutherland Nov 8, 2018 • edited Loading

Choose a reason for hiding this comment

kytrinyx Nov 7, 2018

Choose a reason for hiding this comment

kytrinyx Nov 7, 2018

Choose a reason for hiding this comment

kytrinyx Nov 7, 2018

Choose a reason for hiding this comment

jdsutherland commented Nov 8, 2018 • edited Loading

kytrinyx commented Nov 9, 2018

jdsutherland commented Feb 24, 2019 • edited Loading

kytrinyx commented Feb 26, 2019

jdsutherland commented Feb 27, 2019 • edited Loading

kytrinyx commented Feb 27, 2019

jdsutherland commented Feb 27, 2019

kytrinyx commented Feb 27, 2019

jdsutherland commented Feb 27, 2019

kytrinyx commented Feb 27, 2019

jdsutherland commented Feb 28, 2019 • edited Loading

kytrinyx commented Feb 28, 2019

jdsutherland commented Feb 28, 2019 • edited Loading

kytrinyx commented Feb 28, 2019

kytrinyx commented Feb 28, 2019

kytrinyx commented Feb 28, 2019

jdsutherland commented Feb 28, 2019

jdsutherland commented Mar 7, 2019

jdsutherland commented Oct 26, 2018 •

edited

Loading

jdsutherland commented Nov 3, 2018 •

edited

Loading

jdsutherland Nov 8, 2018 •

edited

Loading

jdsutherland commented Nov 8, 2018 •

edited

Loading

jdsutherland commented Feb 24, 2019 •

edited

Loading

jdsutherland commented Feb 27, 2019 •

edited

Loading

jdsutherland commented Feb 28, 2019 •

edited

Loading

jdsutherland commented Feb 28, 2019 •

edited

Loading