Fallback to default solution.js if locale subdir hasn't got one

[tdd]

Hey guys,

I just realized I sometimes have a use case where an exercise needs per-locale files / display / output confirmation, but the solution.js code itself is unchanged. For instance, see https://github.com/koajs/kick-off-koa/tree/master/exercises/response_body

In order not to duplicate the solution.js file across locales, we should fallback to solution/'s file when the locale-specific solution_xx subdir doesn't provide one. Here's a simple patch for that.

It would be awesome if 1+ contribs (e.g. @martinheidegger or @rvagg or @julianduque) could :+1: this, merge, and version bump on npm so I can change the minimum dep on the relevant workshops :wink:

Cheers!

[martinheidegger]

I think you are trying to add a feature for a feature that might be just broken (even though it doesn't look like it). The behaviour of the code before this PR is (should be):

1.) Test (with fs.exists) if there is a folder with the solutions in an actual language solution_{lang} 2.) Fallback to regular solution folder if it doesn't exist.

This patch seems to would come in handy if someone goes through the pain and create a solution_{lang} folder but wants it explicitly not used. Which kind-of doesn't make sense to me.

[tdd]

Hey Martin,

Well, not quite. I probably failed to convey the use case correctly. I agree it's sort of a blend here, as in the case of Kick Off Koa, it also relies on a custom exercise runner in their workshop, that can lookup output description files within per-locale folders.

Our current infrastructure looks up solution files not in solution_xx.js, but in solution_xx/solution.js, which lets us bundle other per-locale files in there. But often only extraneous files get localized, and the main solution code remains unchanged. So we find ourselves with per-locale dirs, but a single, default-dir solution.js.

Also, the way I understand your proposed approach, the "in an actual language" doesn't seem to honor the active locale. My PR does exactly what you describe, but sticking to the current locale before falling back to default. I'm obviously not interested in the Chinese version of solution.js when there's no variation for it in FR.

Is that clearer?

[martinheidegger]

Oh! I see what you are doing. The recursion made it hard for me to read. The logic makes sense to me. I would suggest to rewrite it though to not expose the "useDefault" flag. Something like:

exercise.getSolutionFiles = function (callback) {
   this._getSolutionFiles('./solution_' + this.lang, function (err, list) {
     if (list && list.length > 0)
       return callback(null, list)
     this._getSolutionFiles('./solution', callback)
   })
})

might be better readable?!

[tdd]

Hey Martin,

That's a nice approach, but I didn't see the usefulness of exposing the core checking logic as a "private" (underscore-prefixed) method, so I kept it as an inner function.

I also put the two path variants in aptly-named variables upfront, so the semantics of the code is immediately apparent.

How about now?

[martinheidegger]

Okay to merge for me. I would give bonus points if the checkPath method would not filter the list ;)

[tdd]

Hey Martin,

Well, the check we perform (which is there in the original code) is about verifying there is a .js file in there. Otherwise it just won't work when we have per-locale files but not a solution file.

Even here, we're only checking for *.js, when solution.js might be a much better idea, leaving other, exercise-needed JS files up for grabs.

[martinheidegger]

Got it, sorry for the misunderstanding. LGTM (and will merge it now)