scotch/sapling — pull request #1 by DjebbZ (2012-09-03 15:47:29+00:00)
Title: Converting sapling to plain Javascript.

Hi Kyle,

**tl;dr** : I converted files to JS. See comments in the last paragraph.

As [I said in the mailing-list](https://groups.google.com/d/msg/angular/O_3mlKiW-OQ/__RCcdqUrcgJ), I started to convert Sapling from CS to JS. It helps me understand CS, JS, Angular and even Brunch, so I think it's a good exercise.

First I want to thank you for creating this project, I'm really interested in learning AngularJS and sapling seems a good starting point to understand the framework deeply.

Here's my methodolgy : a console running the watch script and another committing changes for each file. At the very beginning the watch started complaining about missing `]`s, as well as my IDE (WebStorm). It seems that there's a problem with the parameters passed to the factories. The fact that the last parameter of the array of dependencies is a function is producing errors, but I wasn't able to fix them. They may even be false negatives, since the build scripts worked anyway.

So far I've converted all js in `app/ ` and `app/scripts`. The scripts in the subfolders remain to be done. I'll tackle them tomorrow. I couldn't test them though, I don't have Go installed and know nothing about Go or Python. So my work is mainly CS to JS conversion, with documentation reading and comments adding. I deleted each coffee file and Brunch didn't complain. I manually checked the output app.js file and it looks OK.

I tried to run the tests with `npm test`, but it complained that `error (config): Config file does not exist!`. Indeed the test scripts expect a `test/testacular.config.js` file but it's named `test/testacular_conf.js`. Is there a reason ?

During the conversion I wrote some comments/questions, here they are. Your input would be very much appreciated.

1. in app.services, I think it's possible to use .value('version', '0.1') instead of .factory(…) since it's a constant and not a computed value.
1. the directives 'appVersion' doesn't seem to be used anywhere in the template files (I also search for 'app-version'). It appears just in the tests.
1. shouldn't the getClass() function of the AppCtrl be a directive instead ? My understanding is that a controller shouldn't do DOM manipulation. I know that the controller is just providing a helper for child templates, but it bothers me to see DOM related code in the controller. If I understand directives properly, the directive could use the AppCtrl and change the class based on the $scope.activeId model.
1. the AppCtrl isn't making use of $resource and $rootScope. Are they really necessary ?