yellow team mailing list archive

[Gary Poster <gary.poster@xxxxxxxxxxxxx>]

Land with changes (from both me and Benji).

Thank you Nicola.  This will be very nice to have.

I talked with Benji about this.  He is OK with you landing this with the
requests I've made, and considerations of your other comments.  He also
would really like to reduce the number of constants, but is willing to
be flexible about it and leave the decision to you.

I saw that you ran tests on debug, rather than devel, which is a good
choice to my mind.  I filed
https://bugs.launchpad.net/juju-gui/+bug/1087107 for production.  This
was a pre-existing problem.

I don't like the filesystem division between app ("devel") and debug but
I respect your decision and the logic behind it.  Let's proceed.

I wondered if the help text would be improved by clarifying the meaning
of the different "run levels."  See what you think:
http://pastebin.ubuntu.com/1413894/ .

Thanks again,

Gary


https://codereview.appspot.com/6878053/diff/5001/.bzrignore
File .bzrignore (right):

https://codereview.appspot.com/6878053/diff/5001/.bzrignore#newcode16
.bzrignore:16: build*
On 2012/12/05 20:37:17, benji wrote:
> This ignore is too broad.  It will ignore all files which have names
that begin
> with the string "build".
+1

https://codereview.appspot.com/6878053/diff/5001/Makefile
File Makefile (right):

https://codereview.appspot.com/6878053/diff/5001/Makefile#newcode35
Makefile:35: PROD_ASSETS=$(PROD)/$(JUJU_UI)/assets
On 2012/12/05 20:37:17, benji wrote:
> These sorts of aliases make the code much harder for me to read.  For
example, I
> know what "app" is, hiding it behind "SRC" just obfuscates things.
I have more sympathy for constants like this than benji does when in
Python land, because I like NameErrors to find my typos.  Make doesn't
give us that though.  The BUILD, DEBUG, PROD variables don't seem to add
much to me in particular.  I shrug, even though benji doesn't.

https://codereview.appspot.com/6878053/diff/5001/Makefile#newcode156
Makefile:156: link_debug_files:
On 2012/12/05 20:37:17, benji wrote:
> Wow, that is a lot of links.  I guess this means that every time we
add a source
> file we will have to add a line here, right?  Is it not possible to
just serve
> the files directly from app/ and avoid all of this?  Or
programmatically
> generate the links instead of having to enumerate them all?

I agree that these are a lot of links.  However, it is not nearly as bad
as benji fears, and essentially the same as what we have now: as long as
we add files only in models, store, templates (via handlebars building),
views, widgets, images, javascripts, and svgs, then we will not need to
change this list.

I am unhappy with this list for two other reasons, though.

First, you are moving things around from the app directory.  If the app
directory mirrored the expected deployment structure as I had suggested,
wouldn't this have been simpler, shorter, and possibly more amenable to
generating links as benji describes?  REQUESTED ACTION: either change
the app directory to have the desired structure or make a bug to do so
later (and then remove as much as possible the need to identify
individual files here).

Second, you are maintaining both link_debug_files and link_prod_files.
As far as I can tell on a quick skim, link_debug_files is a rough
superset of the actions of link_prod_files.  REQUESTED ACTION: Could you
make a macro for the shared bits
(http://www.gnu.org/software/make/manual/html_node/Call-Function.html)
so we reduce the chance of drift?

https://codereview.appspot.com/6878053/diff/5001/Makefile#newcode158
Makefile:158: ln -sf `pwd`/$(SRC)/favicon.ico `pwd`/$(DEBUG)/
On 2012/12/05 20:37:17, benji wrote:
> The Makefile finds the current working directory two ways, $(PWD) and
`pwd` as
> here.  $(PWD) is slightly better because `pwd` runs a shell each time
just to
> get the current directory.
+1

https://codereview.appspot.com/6878053/

-- 
https://code.launchpad.net/~teknico/juju-gui/bug-1078910-serve-app-statically/+merge/138212
Your team Juju GUI Hackers is requested to review the proposed merge of lp:~teknico/juju-gui/bug-1078910-serve-app-statically into lp:juju-gui.