|
|
|
Ben's branch for env refactoring
Ben's branch for env refactoring... testing
https://code.launchpad.net/~hazmat/juju-gui/component-modules/+merge/135501
(do not edit description out of merge proposal)
Patch Set 1 #
Total comments: 17
Total messages: 3
|
hazmat
Please take a look.
|
13 years, 1 month ago (2012年11月21日 19:25:59 UTC) #1 | |||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
Please take a look.
I do not understand the intent of this branch, so my opinions can not realistically considered a review, but I noticed several small issues and commented on them. A larger issue is that this branch is quite a bit bigger than our target diff size. It would be nice to break it up into a few smaller branches. Also, for the volume of code, there are relatively few tests. Smaller branches would be easier to test convincingly, I think. https://codereview.appspot.com/6842084/diff/1/Makefile File Makefile (right): https://codereview.appspot.com/6842084/diff/1/Makefile#newcode100 Makefile:100: lint: gjslint jshint Was this intentional? https://codereview.appspot.com/6842084/diff/1/app/assets/javascripts/d3-compo... File app/assets/javascripts/d3-components.js (right): https://codereview.appspot.com/6842084/diff/1/app/assets/javascripts/d3-compo... app/assets/javascripts/d3-components.js:352: * @method renderOnce The normal YUIDoc style is to have the prose first. Also, we have been including @return even if it returns nothing so that we can distinguish between "returns nothing" and "hasn't been documented". https://codereview.appspot.com/6842084/diff/1/app/assets/javascripts/d3-compo... app/assets/javascripts/d3-components.js:356: renderOnce: function() {}, "renderOnce" sounds more like a directive ("this thing should only be rendered once"). Maybe "renderFirst" would be better. Or, perhaps the regular render function should be called with a boolean indicating whether it is the first time. https://codereview.appspot.com/6842084/diff/1/app/views/environment.js File app/views/environment.js (right): https://codereview.appspot.com/6842084/diff/1/app/views/environment.js#newcode5 app/views/environment.js:5: * @module environment I think this was correct before. Maybe something else changed that I am not seeing. @module is about the logical namespace, not the file organization on disk. That may be what is confusing things here. Since this file defines things that go in the juju.views namespace, @module should be "views". https://codereview.appspot.com/6842084/diff/1/app/views/environment.js#newcode19 app/views/environment.js:19: * @namespace juju.views The YUIDoc style guide says that the "root" namespace ("juju" in our case) should be excluded from the @namespace directive, so no "juju." here. https://codereview.appspot.com/6842084/diff/1/app/views/topology/panzoom.js File app/views/topology/panzoom.js (right): https://codereview.appspot.com/6842084/diff/1/app/views/topology/panzoom.js#n... app/views/topology/panzoom.js:12: **/ Ditto YUIDoc notes from earlier. https://codereview.appspot.com/6842084/diff/1/app/views/topology/service.js File app/views/topology/service.js (right): https://codereview.appspot.com/6842084/diff/1/app/views/topology/service.js#n... app/views/topology/service.js:139: .attr('width', function(d) { We have been indenting chained method calls in other parts of the code. https://codereview.appspot.com/6842084/diff/1/app/views/topology/service.js#n... app/views/topology/service.js:345: An extra newline or two snuck in here. https://codereview.appspot.com/6842084/diff/1/app/views/topology/topology.js File app/views/topology/topology.js (right): https://codereview.appspot.com/6842084/diff/1/app/views/topology/topology.js#... app/views/topology/topology.js:82: **/ The pickiest of knits: YUIDoc comments of the "stars down the side" form usually end in just a single asterisk. https://codereview.appspot.com/6842084/diff/1/test/test_topology.js File test/test_topology.js (right): https://codereview.appspot.com/6842084/diff/1/test/test_topology.js#newcode51 test/test_topology.js:51: '</div>'); We should use YUI's node building instead of including HTML snippets in code. The style guide has more details.
This review is made by me and Nicola. Looking at the code we think we understood the logic of it all. This split of the env view into components and modules seems good, and allows composing the resulting scene in a more modular way. However, a change of such scope and size really requires to be documented both in the code and in the project documentation. Your comment in this MP is a good start, and needs to be pushed into the docs and expanded/better explained. Furthermore, we agree with Benji on the need for quite a few more tests. Given the size of this proposal, it would be helpful to split this branch up into a series of smaller changes. Finally, we only see two tests running (without failures). Inline comments follow. https://codereview.appspot.com/6842084/diff/1/Makefile File Makefile (right): https://codereview.appspot.com/6842084/diff/1/Makefile#newcode100 Makefile:100: lint: gjslint jshint On 2012年11月21日 20:43:27, benji wrote: > Was this intentional? Yeah, why did it have to go? https://codereview.appspot.com/6842084/diff/1/app/assets/javascripts/d3-compo... File app/assets/javascripts/d3-components.js (right): https://codereview.appspot.com/6842084/diff/1/app/assets/javascripts/d3-compo... app/assets/javascripts/d3-components.js:97: if (ModClassOrInstance === undefined) { Is this check here for some specific need, or is it some kind of debug leftover? https://codereview.appspot.com/6842084/diff/1/app/assets/javascripts/d3-compo... app/assets/javascripts/d3-components.js:364: * and update(). If update requires some render state to operate on Please append a comma to this line. https://codereview.appspot.com/6842084/diff/1/app/views/environment.js File app/views/environment.js (right): https://codereview.appspot.com/6842084/diff/1/app/views/environment.js#newcode31 app/views/environment.js:31: topo; topo in Italian means mouse ;-) https://codereview.appspot.com/6842084/diff/1/app/views/topology/topology.js File app/views/topology/topology.js (right): https://codereview.appspot.com/6842084/diff/1/app/views/topology/topology.js#... app/views/topology/topology.js:9: * Topology models and renders the SVG of the envionment topology typo: environment. https://codereview.appspot.com/6842084/diff/1/app/views/topology/topology.js#... app/views/topology/topology.js:23: options = options || {}; This line is a bit confusing. Isn't ``options`` a local var? Are these ``options`` used anywhere? https://codereview.appspot.com/6842084/diff/1/app/views/topology/topology.js#... app/views/topology/topology.js:27: render: function() { Is this method override required? It seems to just invoke the superclass one without adding anything.