Code review comment for lp://staging/~gary/juju-gui/sandboxEnv-2

On 03/12/2013 08:59 AM, Brad Crittenden wrote:
> LGTM modulo a couple of changes.
>
> Before we'd not done any YAML parsing in JS due to Kapil's assertion
> that there were no libraries that were acceptable. Is the one you've
> chosen suitable?

I think it is. As I mentioned in my cover letter, this is a new 2.x
rewrite of the recommended JS yaml parser. I believe it has been
released since Kapil's last evaluation. It parsed the juju-gui yaml
docs I had with flying colors, and the example the project gives
(http://nodeca.github.com/js-yaml/) looks good too.

> Can it be used in the parsing of config files when the
> user uploads one?

Yes, I think it could be.

> If so, that would make passing YAML obsolete and
> would give a better user experience.

Definitely agreed. However, I think we can use what I've done here as a
way to get our toes wet first. If this works out well, then we ought to
plan the change.

> https://codereview.appspot.com/7741043/diff/1/app/store/env/fakebackend.js
> File app/store/env/fakebackend.js (right):
>
> https://codereview.appspot.com/7741043/diff/1/app/store/env/fakebackend.js#newcode272
> app/store/env/fakebackend.js:272: }
> Originally improv accepted both and just gave precedence to configYAML.
> The go implementation does the same so it would be nice for this
> implementation to be consistent.

Ah, thank you. I didn't know that the Go version behaved identically,
but you would. :-) Thanks, I changed this.

>
> https://codereview.appspot.com/7741043/diff/1/test/test_fakebackend.js
> File test/test_fakebackend.js (right):
>
> https://codereview.appspot.com/7741043/diff/1/test/test_fakebackend.js#newcode200
> test/test_fakebackend.js:200:
> This test will need to be fixed if you make the previously suggested
> change.

Done.

Thanks to both you and Nicola!

Gary