yellow team mailing list archive

[Thiago Veronezi <thiago@xxxxxxxxxxxx>]

Thanks Gary for your review!


https://codereview.appspot.com/6844061/diff/1/.bzrignore
File .bzrignore (right):

https://codereview.appspot.com/6844061/diff/1/.bzrignore#newcode16
.bzrignore:16: build
On 2012/11/19 19:00:33, gary.poster wrote:
> Nice simplification and clean-up.

Ohhh yeah. :O)

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

https://codereview.appspot.com/6844061/diff/1/Makefile#newcode100
Makefile:100:
On 2012/11/19 19:00:33, gary.poster wrote:
> On 2012/11/19 18:18:58, bac wrote:
> > I'm concerned about separate build and install targets and having
template.js
> in
> > a separate rule from the others.  Seems complicated and twisty.

> Having "build" depend on "install" also seems odd, conceptually.

> Does the "install" target bring any value?  That is, is there any use
case for
> running it alone?  If not, I suggest combining it and "build" into the
"build"
> target (and deleting the "install" target).  What do you two think of
that?

Ah... I see! I will delete the "install" target and change the "build"
target from...

build: install

... to...

build: appcache $(NODE_TARGETS) build/juju-ui/templates.js yuidoc
spritegen combinejs

Is that ok?

https://codereview.appspot.com/6844061/diff/1/lib/server.js
File lib/server.js (right):

https://codereview.appspot.com/6844061/diff/1/lib/server.js#newcode57
lib/server.js:57: res.sendfile('build/juju-ui/assets/' + fileName);
On 2012/11/19 19:00:33, gary.poster wrote:
> Interesting.  I wanted the development server code to be even simpler
than this,
> but I can see why what you did is compelling.  I would be tempted to
experiment
> with converting the js files to symlinks in a later branch and
simplifying this
> code.  This is an improvement anyway, though, and maybe my hunch won't
work out.

I needed to change the "test-server.js" file too. The tests were broken
due to the new notification stuff. I've tried to use the symbolic link
approach in order to avoid it, but with no success.

I will use the "new branch" option to do it. Thanks.

https://codereview.appspot.com/6844061/

-- 
https://code.launchpad.net/~tveronezi/juju-gui/make-build/+merge/134706
Your team Juju GUI Hackers is requested to review the proposed merge of lp:~tveronezi/juju-gui/make-build into lp:juju-gui.