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. -- Mailing list: https://launchpad.net/~yellow Post to : [email protected] Unsubscribe : https://launchpad.net/~yellow More help : https://help.launchpad.net/ListHelp
