[WIP] Add doctor cmd - migrate legacy exercises #744
Conversation
* Passing * Copied most of download#runDownload. Payload download seems like it needs extracted out of cmd/download.
jdsutherland
force-pushed
the
add-doctor-cmd
branch
from
c2b2f77
to
97f0b5f
Compare
|
|
||
| func setupDoctorFlags(flags *pflag.FlagSet) { | ||
| flags.BoolP("fixup", "f", false, "run tasks") | ||
| // TODO: --dry-run for fixup |
There was a problem hiding this comment.
I think we don't need both --dry-run and --fixup. Originally I was thinking that running doctor bare would fix things, and in that case we'd need --dry-run to just do reporting. With this approach we're reversing the logic, so that doctor bare does the reporting, which means that we need a flag (--fixup) to make the actual changes.
|
|
||
| // TODO: extract this and cmd/download into download service? | ||
| // copied from download#runDownload | ||
| func downloadMetadata(exercise workspace.Exercise, cfg config.Config) error { |
There was a problem hiding this comment.
unexported functions are available in the entire cmd package. We don't need to copy it, just extract it.
There was a problem hiding this comment.
Right. I wasn't suggesting that it was necessary to duplicate the function within the same package. I was trying to point out that runDownload currently has duplicate behavior that we're going to want to use in doctor but we can't use the existing function (because we only need metadata whereas this function also writes all solution files). It seems that the logic for getting a downloadPayload should be extracted outside of the cmd package (given that multiple cmd's use it) with separate functions for writing metadata and solution files. I'm wondering if #719 is related.
If #719 doesn't cover this, where would you suggest these extractions live?
There was a problem hiding this comment.
I'm not sure that it should live outside the cmd package, if only commands need it.
The code in api is very generic, whereas this is pretty specific.
I'd stick the extracted logic in cmd/cmd.go for now.
There was a problem hiding this comment.
Only the commands need it but it seems more like a service than an actual command and currently everything in the cmd package is a command. It also seems conceivable that something other than cmd might want to consume it.
The download payload stuff could go in cmd/cmd as a tacit home for shared command functionality or it could be put into a new 'helper/util' file. Maybe that's fine but it seems like it would be an outlier, so that's the rationale for considering putting it outside.
I gave a shot at the refactoring in #756
| fixup, err := flags.GetBool("fixup") | ||
| if err != nil { | ||
| return err | ||
| } |
There was a problem hiding this comment.
In terms of design, what I think would help make it easy to do both modes (reporting and fixing), would be to have a type for each type of thing we're fixing. We can initialize the value, and have three different steps:
- Investigate.
- Report.
- Fix.
The investigation would store whatever it needs to make it easy to both report and fix.
jdsutherland
changed the title
|
I don't anticipate completing this so I'm going to close it. |
|
Ok, thank you @jdsutherland. |
Closes #639
Closes 699- decided clean up numeric suffixes should be split into separate PR.Still a rough WIP.