Changes exercism fetch behavior to remove filesystem noise

[devonestes]

This commit changes the behavior of exercism fetch when called without any arguments. Instead of fetching and writing exercises for all tracks, it will now fetch and update only tracks for which the user has folders in their local environment. If a user calls exercism fetch without having any language tracks in their exercises folder, a prompt is shown to the user to tell them how to list all available tracks and how to fetch their first track for a language of their choice.

[devonestes]

This is my rough first attempt at a fix for #226. Any feedback is much appreciated!

[kytrinyx]

A couple things (philosophically):

1. I think we should only use os.Exit for a value other than 0 (in the case of an error), and
2. I don't think we should ever have os.Exit in packages other than cmd. Other packages should return errors, if that's what they are.

[kytrinyx]

I like where this is going. A few suggestions, but nothing major, I think.

[devonestes]

@kytrinyx thanks for the feedback! All your comments have been addressed, and I made a couple other small changes to be more consistent with the existing style in homework.go.

[kytrinyx]

Instead of LANGUAGE this should be TRACK_ID (it's super confusing, I'm so sorry!).

Should we return that as an error instead of printing it here?

[devonestes]

I didn't really think that this warranted an error, since when an error is returned from this that will bubble up to a log.Fatal call and exit in that fashion. I thought that might be a little jarring for the user, so I decided to go this route. However, I'm happy to change this to an error if you think that's more intention revealing!

[kytrinyx]

Hm. Yeah. I see what you mean.

I think it is an error, though. They asked to fetch something, and we're telling them: something's not right.

[devonestes]

That's a good point. If the user asked to do something and it couldn't be done, then that's totally an error. Ok, switching over to an error now!

[kytrinyx]

This is looking really clean. I'm worried about that one name, but that's it.

[devonestes]

@kytrinyx I think we're nearly there. I still don't love the name of isFetchedWithoutArgs, but until someone can think of something better that's what there. Maybe something will come to me after a good night's sleep!

[kytrinyx]

I like filterMode better than isFetchedWithoutArgs, to be honest. filterMode is about what we're deciding (to filter or not) whereas isFetchedWithoutArgs is one step away. The reader has to make the connection between calling without arguments and this meaning that we're going to filter.

[devonestes]

I sort of like the isSomething name. filterMode is a great name when it's next to an if, but outside of that context it seems a little odd to me. How about shouldFilter?

[kytrinyx]

Yeah I like the is... thing better, too. shouldFilter is better, and it reads better than filterMode in the context as well. Let's go with that. If someone thinks of something better, holler!

[devonestes]

@kytrinyx Great, shouldFilter it is! Let me know if there's anything else that jumps out at you that you think should be changed and I'll get right on it.

[lcowell]

Looks great. DoesRejectMissingTracks need to be exported? It makes sense to have SaveAfterFetch exported because it's the replacement entry point for Save.

I think it would be good to see a test to verify this new behaviour. It could to be a little difficult because you're calling os.Stat deeper in your code. I'm wondering if we should pass a list of known language to SaveAfterFetch, so that we don't have to set up a whole directory in order to test this.

[devonestes]

@lcowell Good call on not exporting RejectMissingTracks!

I agree that a couple of tests would be a good thing, but I think the list of known languages would be a real hassle to keep up to date. Dealing with creating and deleting directories for the tests won't be that bad, so I'll just go that route for now.

[lcowell]

To get the list of user languages, isn't it just a matter of getting a listing of all the directories in your exercism language? Something like:

filepath.Glob(filepath.Join(exercismDir, "*"))

[devonestes]

@lcowell Sorry, I thought when you said 'Pass a list of known languages' you meant that we'd keep a list of the available languages on exercism.io in a constant or something and pass that list. My misunderstanding!

Yes, we could totally go the way you describe to solve this problem (that's the way I originally thought of doing it), but it was faster and easier for me to write the tests with the code in the shape it is in now rather than refactoring. If there's a particular problem or reason why it would be better to refactor then I'd love to hear it and would be glad to refactor!

[lcowell]

I think the motivation is to isolate the parts of the system that need to talk to the OS. This improves testability and reduces brittleness in the system. In a system where everything can talk to everything it becomes more difficult to reason about what's happening or what went wrong.

[kytrinyx]

@lcowell excellent call on passing the list of languages to SaveAfterFetch() so we can do an isolated test on that. @devonestes Do you see how that would work?

[devonestes]

@kytrinyx @lcowell Your reasoning makes total sense - the separation of concerns benefit is definitely worth a refactor. I'll get on in two shakes of a lamb's tail 🐑

[devonestes]

Ok, let me know what you think about the way I've refactored this, and thanks again for the idea!

[lcowell]

This looks great. Is there some reason we can't handle shouldFilter in SaveAfterFetch? If we do that rejectMissingTracks will do exactly what it says it does and we avoid passing an argument where we don't need to. I think passing shouldFilter in made more sense when rejectMissingTracks was exported.

[devonestes]

@lcowell No reason, really. @kytrinyx had recommended that structure in a previous comment (here: #258 (comment)), so I went with that since it didn't really bother me either way.

[kytrinyx]

I like @lcowell's suggestion better :) (sorry!)

[devonestes]

@kytrinyx @lcowell No worries - that's why more opinions are better than one!

[devonestes]

Ok, updated and ready for review!

[lcowell]

Looks great. Nice work @devonestes !

Edit: I do feel like SaveAfterFetch says when it does it, but it doesn't really describe what it does. Any thoughts on a name like FilterAndSave instead?

[kytrinyx]

Agreed, now that you mention it :) Gawd I love code review.

Now that you mention it, should we just have one Save() function and one Reject or Filter function that we call from the cmd package? (I'm so sorry to keep thinking out loud about this, it really does look good!).

[lcowell]

I like code review too. I learn so much, whether I'm the one giving feedback or the one writing code.

In the context of homework, which is something that is submitted, I think that Filter makes more sense than Reject.

I see a couple of approaches to implementing this:

  hw.Filter(dirMap)
  err := hw.Save()

or if we have Filter return a copy of the homework:

  err := hw.Filter(dirMap).Save()

[kytrinyx]

Yepp, I agree.

[devonestes]

Well right now in homework we have the following code:

// HWFilter is used to categorize homework items.
type HWFilter int

// SummaryOption allows selective display of summary items.
type SummaryOption HWFilter

const (
    // HWAll represents all items in the collection.
    HWAll = iota
    // HWUpdated represents problems where files have been added.
    HWUpdated
    // HWNew represents newly fetched problems.
    HWNew
    // HWNotSubmitted represents problems that have not yet been submitted for review.
    HWNotSubmitted
)

...

// ItemsMatching returns a subset of the set of problems.
func (hw *Homework) ItemsMatching(filter HWFilter) []*Item {
    items := []*Item{}
    for _, item := range hw.Items {
        if item.Matches(filter) {
            items = append(items, item)
        }
    }
    return items
}

Since there's already a bit of framework for filtering hw.Items, should I try and integrate this filtering in with that? I avoided that at first since it seemed like it was more hassle than it was worth, but if we think that might be helpful I'll take a crack at it.

[lcowell]

Can you briefly describe how would you implement this? The only ways I can think of would add complexity instead of reduce. But hey, I'm only one coffee deep, so I might be missing something obvious.

[devonestes]

@lcowell I totally agree that it would add complexity, but if the goal is to have one Save() function and one Filter() function, then I don't think that this code can exist alongside the single Filter() function without causing confusion. My concern is that by generalizing the names and not incorporating all of the behavior that might fall under those names, that maintainers later on will get confused.

[kytrinyx]

I don't want to add complexity.

Here's what I was thinking:

// in cmd/fetch.go
if len(ctx.Args()) == 0 {
  if err := hw.FilterMissingTracks(dirMap); err != nil {
    log.Fatal(err)
  }
}
if err := hw.Save(); err != nil {
  log.Fatal(err)
}

[devonestes]

Ahh, I understand now. Crystal clear - I'll get on it now!

[kytrinyx]

I really like how this ended up. Thanks @devonestes and @lcowell for working through all the details!