Code review comment for lp:~gary/juju-gui/grabbag

Revision history for this message
Francesco Banconi (frankban) wrote :

Land with changes.

This review is done by me and Nicola.
This branch looks great Gary, it solves a lot of problems.
The only one issue we found is the presence of failures in test-debug,
but below we are suggesting a solution.
Thanks also for the documentation clean up (especially the how to make
releases part).
Nicola says: special thanks for renaming the build-devel target, you da
man! :-)

https://codereview.appspot.com/7005044/diff/1/Makefile
File Makefile (left):

https://codereview.appspot.com/7005044/diff/1/Makefile#oldcode455
Makefile:455: .PHONY: appcache-force appcache-touch beautify build \
Isn't build a phony target now?

https://codereview.appspot.com/7005044/diff/1/Makefile
File Makefile (right):

https://codereview.appspot.com/7005044/diff/1/Makefile#newcode32
Makefile:32: -e '^app/assets/javascripts/gallery-.*\.js$$' \
Really nice, thank you.

https://codereview.appspot.com/7005044/diff/1/app/modules-debug.js
File app/modules-debug.js (right):

https://codereview.appspot.com/7005044/diff/1/app/modules-debug.js#newcode29
app/modules-debug.js:29: fullpath:
'juju-ui/assets/javascripts/gallery-timer-debug.js'
These three paths need to be absolute (add a slash at the beginning of
each one). Otherwise, in test-debug runs, they are searched inside the
/test/ path, and a bunch of tests fail.

https://codereview.appspot.com/7005044/diff/1/app/views/utils.js
File app/views/utils.js (right):

https://codereview.appspot.com/7005044/diff/1/app/views/utils.js#newcode121
app/views/utils.js:121: debug: noop
Yay! No more errors running test-prod!

https://codereview.appspot.com/7005044/

« Back to merge proposal