workshopper / workshopper-exercise

An exercise runner component for the Workshopper framework
Other
57 stars 35 forks source link

What is the point of solution dir & translated solutions? #19

Open Sequoia opened 9 years ago

Sequoia commented 9 years ago

I'm creating a new workshopper on Workshopper 2.3.1. In creating my first exercise I came across the solution dir. I'm using learnyounode for examples so I checked what was in these directories.

$ ls */solution
baby_steps/solution:
solution.js

filtered_ls/solution:
solution.js

hello_world/solution:
solution.js

http_client/solution:
solution.js

http_collect/solution:
solution.js

http_file_server/solution:
solution.js

http_json_api_server/solution:
solution.js

http_uppercaserer/solution:
solution.js

juggling_async/solution:
solution.js

make_it_modular/solution:
solution.js        solution_filter.js

my_first_async_io/solution:
solution.js

my_first_io/solution:
solution.js

time_server/solution:
solution.js

It didn't seem to be that there was much in these dirs besides a solution.js.

I checked to find out what was going on & saw that it's for translated solution files. This confused me because I wondered what would need translating in a solution file. I found some translated solutions in my_first_io & took a look: ==> solution/solution.js <==

var fs = require('fs')

var contents = fs.readFileSync(process.argv[2])
var lines = contents.toString().split('\n').length - 1
console.log(lines)

// note you can avoid the .toString() by passing 'utf8' as the
// second argument to readFileSync, then you'll get a String!
//
// fs.readFileSync(process.argv[2], 'utf8').split('\n').length - 1

==> solution_fr/solution.js <==

var fs = require('fs')

var contents = fs.readFileSync(process.argv[2])
var lines = contents.toString().split('\n').length - 1
console.log(lines)

// remarquez que vous pouvez éviter d’avoir à appeler `.toString()`
// en précisant un encodage 'utf8' comme second argument pour
// `readFileSync`, ce qui vous renverrait une `String` !
//
// fs.readFileSync(process.argv[2], 'utf8').split('\n').length - 1

==> solution_ja/solution.js <==

var fs = require('fs')

var contents = fs.readFileSync(process.argv[2])
var lines = contents.toString().split('\n').length - 1
console.log(lines)

// 注:'readFileSync' の二つ目の引数に 'utf8' を渡すと、
// '.toString' を使わずに文字列を受け取ることが出来ます!
// fs.readFileSync(process.argv[2], 'utf8').split('\n').length - 1

The only thing translated seems to be... the comments? :confused: I looked further to see if I could find the exercise that needed to have translated versions of the solution. ==> solution/solution.js <==

console.log("HELLO WORLD")

==> solution_fr/solution.js <==

console.log("Bonjour, Monde");

==> solution_ja/solution.js <==

console.log("こんにちは世界")

:question: :grey_question: :question: There's an i18n framework in place for this.

My Point

I believe that the "translated solutions" is a :-1: bad idea. Here's why:

  1. In introduces a ton of unneeded maintenance work. Each solution must now be maintained separately for every language supported. If someone submits a patch against an en solution file, do you hold the PR & tell them to go update all 7 or however many translation files?
  2. It introduces a big entrance for bugs. What if the en solution is patched & the others are overlooked because maintainer only speaks en? Now the solution behavior has diverged by language.
  3. It makes the translator's job way more complicated. Say I patch a solution & add a comment in English, now in addition to lang.json & problem_lang.md, the translators need to comb thru every exercise looking for untranslated/updated comments? :frowning:
  4. I can't imagine the scenario where a solution should need to be different by language. I'm sure someone could be contrived but do they exist in the wild? I strongly suspect that if you need different solutions per language you are probably doing something wrong. If this corner-case should occur, is there any reason the author can't handle it (switch on languages) in the solution.js?
  5. Regarding comments: solution.js is not the place to put exercise hints! How would the average nodeschooler even know to look here? If // note you can avoid the .toString() by passing 'utf8' as the second argument to readFileSync, then you'll get a String! is important for people to read, put it in the problem.md where people can find it.

:question: What problem is this feature solving?

Proposal

Load exercises/my_foo/solution.js & call it a day. I will try to submit a PR given the green light.

Why I even care about this

Because I'm authoring a workshopper & it's bothersome to me, but more importantly, stuff like this needlessly makes it more difficult & complex to create a workshopper! The new workshopper & workshopper-excercise APIs have introduced a lot of complexity & I am arguing for the potential author who just wants to make a new workshopper easily.

martinheidegger commented 9 years ago

Thank you for taking the time to write this up. Actually I am available at gitter that should have cleared a lot of questions. There are two reasons for the current structure:

  1. The reason why its a folder is: Some solutions have more than one file and it would it is nice to be able to copy the whole folder.
  2. The translations are entirely optional and exist only because someone decided to write documentation into the solution files. I preferred they wouldn't be there but I search for a quick solution and this was the only thing that I saw okay without breaking the current content.
Sequoia commented 9 years ago

@martinheidegger Thx for reply. What problem does this solve?

Can you point to any other project that maintains multiple copies of its codebase, one per language?

Sequoia commented 9 years ago

This decision is up to @rvagg & whoever else ultimately so I'll leave it alone, but I want to make clear that I'm not saying this feature is a superfluous nice-to-have, I'm saying this feature is a bug. A feature that encourages users to...

  1. duplicate their code into multiple, almost usually identical javascript files and
  2. bury lesson documentation & hints away from where most users will find them

is a bad feature & damages the client code written on the framework. The only rationale for this feature I've seen so far is what you write above:

someone decided to write documentation into the solution files

If in fact it was important lesson content, writing it into the solution file was a bad choice. The framework should not be rewritten to accommodate mistakes like this, especially when it impacts many other people's packages. For example: this is just the English version with different formatting. I assume it's obvious why this is a undesirable. Translators are now reorganizing existing lessons to propagate this mistake into them.

I hope you'll consider the maintenance impact this will have on workshoppers as well as the difficulty encouraging people to hide lesson hints deep in the workshop code will cause for learners! And the fact that there's already an i18n tool to handle string translation. Anyway I'll get back to my nodeschool lesson now, do what you want with this issue. :smile_cat:

Sequoia commented 9 years ago

Working on my lesson-set I see that the solution is printed after a lesson is successfully completed. Is this the place where translation is desired? This makes sense but I still think the maintenance tradeoff is not worth it-- better include a solution.txt like adventure so translations are separate from the source code.

martinheidegger commented 9 years ago

Language specific solution files are discouraged! It is there because the solution will be showed to the user and thus contains "information" to be shown to the user (as you just figured out)