> On April 17, 2014, 12:21 a.m., David McLaughlin wrote:
> > src/main/resources/org/apache/aurora/scheduler/http/ui/js/controllers.js, 
> > lines 361-363
> > <https://reviews.apache.org/r/19833/diff/7/?file=560830#file560830line361>
> >
> >     Better to move these default values to getTasksForJob (is the empty 
> > string default value even required in the Angular templating language?).

The defaults are not required, but not having defaults, makes it hard to 
understand what's in the scope and what the default type should be. Further, 
assigning default values makes the UI more predictable when there is no data. 
So, keeping these.


> On April 17, 2014, 12:21 a.m., David McLaughlin wrote:
> > src/main/resources/org/apache/aurora/scheduler/http/ui/js/controllers.js, 
> > line 403
> > <https://reviews.apache.org/r/19833/diff/7/?file=560830#file560830line403>
> >
> >     Why isn't this part of taskUtil?

Considered that. But left it here since this code is part of the data needed by 
the controller. Summarizing task configs is sort of an independent feature that 
can be used else where so moved it out of this class. Further, the rest of the 
code depends on the object returned here so it makes the controller code easier 
to read.


> On April 17, 2014, 12:21 a.m., David McLaughlin wrote:
> > src/main/resources/org/apache/aurora/scheduler/http/ui/js/controllers.js, 
> > line 365
> > <https://reviews.apache.org/r/19833/diff/7/?file=560830#file560830line365>
> >
> >     Why are you passing $scope.role, $scope.environment and $scope.job into 
> > a function which reads and writes to the $scope object anyway? Why not just 
> > pass $scope? Or why have parameters at all if it's just a closure?
> >     
> >     If the intent is to break a large controller into functions to make it 
> > easier to read (which I think is a good idea), it is best to define this 
> > function outside of the controller itself, so that the function body is 
> > just the control flow.
> >     
> >     I'm not sure of the idiomatic way to do this in Angular.JS, but 
> > normally you just use JavaScript function scope to break your code into 
> > smaller functions without polluting your API. 
> >     
> >     I mean intuitively I would just do something like:
> >     
> >     (function() {
> >       function getTasksForJob($scope) {
> >         // do some side-effecty things to $scope
> >       }
> >     
> >       function jobController($scope, ..) {
> >         // only control-flow code here
> >         getTasksForJob($scope);
> >       }
> >       auroraUIControllers.controller('JobController', jobController);
> >     })();

Passing the required fields explicitly makes the function signature simpler to 
reason about. I don't see much to be gained by moving this functionality out of 
the controller. Since we follow this pattern for other controllers, leaving it 
as is here. I can get rid of the arguments if you want and just access scope 
directly, but that would make it harder to know what args the function needs 
during tests.


> On April 17, 2014, 12:21 a.m., David McLaughlin wrote:
> > src/main/resources/org/apache/aurora/scheduler/http/ui/js/services.js, line 
> > 92
> > <https://reviews.apache.org/r/19833/diff/7/?file=560833#file560833line92>
> >
> >     I realise this code doesn't have bugs right now, but the 'this' keyword 
> > can cause subtle problems given how easy it is to change the calling scope. 
> > You can avoid having to worry about it here by pulling the return object 
> > into a named variable and using that instead.
> >     
> >     i.e.
> >     
> >     function() {
> >       var taskUtil = { 
> >         summarizeActiveConfigs: function(..) {
> >           //... this will still work
> >             taskUtil.isActiveTask(task);
> >           // ..
> >         },
> >         isActiveTask: function(..) { .. }
> >       }
> >       return taskUtil;
> >     }

Yeah this is better. Fixed it in the auroraClient as well.


> On April 17, 2014, 12:21 a.m., David McLaughlin wrote:
> > src/main/resources/org/apache/aurora/scheduler/http/ui/js/services.js, 
> > lines 132-149
> > <https://reviews.apache.org/r/19833/diff/7/?file=560833#file560833line132>
> >
> >     Style:
> >     
> >     Beyond a one liner, it is best to extract these values into temporary 
> > variables.
> >     
> >     i.e.
> >     
> >     var constraints = _.chain(task.constraints) // etc.
> >     
> >     var metadata = _.chain(task.metadata)
> >     // etc.
> >     
> >     return  { 
> >       // ..
> >       constraints: constraints,
> >       metadata: metadata,
> >       // ..
> >     }

We typically avoid spurious variables. In this case I think it's justified.


- Suman


-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/19833/#review40608
-----------------------------------------------------------


On April 16, 2014, 8:37 a.m., Suman Karumuri wrote:
> 
> -----------------------------------------------------------
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/19833/
> -----------------------------------------------------------
> 
> (Updated April 16, 2014, 8:37 a.m.)
> 
> 
> Review request for Aurora, Kevin Sweeney and Bill Farner.
> 
> 
> Bugs: AURORA-281
>     https://issues.apache.org/jira/browse/AURORA-281
> 
> 
> Repository: aurora
> 
> 
> Description
> -------
> 
> *** Please review Diff 2. Diff 1 is https://reviews.apache.org/r/19565/. 
> Added it here so only new code can be reviewed. ***
> 
> 
> Migrated job page to AngularJS.
> Removed old scheduler job page.
> Removed js related to the old scheduler page. Removed data tables.
> Added moment.js library for date time manipulation.
> Refactored code a bit for reusability.
> Stats link should still be added to the new jobs page.
> 
> Since the role/env code is not committed, this diff contains that code as 
> well. 
> Code changes start from page 4 in diff between version 1 and 2.
> 
> 
> Diffs
> -----
> 
>   build.gradle 109c193da3324bd5534b409bfabb6aeb0adda7b1 
>   src/main/java/org/apache/aurora/scheduler/app/AppModule.java 
> eeafc784e915137cacd5f64df1252ccbaf6c0f6c 
>   src/main/java/org/apache/aurora/scheduler/app/SchedulerMain.java 
> ec56c649116c03ef148bac916bd6691a94685bc3 
>   src/main/java/org/apache/aurora/scheduler/http/DisplayUtils.java 
> 19df7889f15b4cf44e386d8ce0626cc94fdcdfba 
>   src/main/java/org/apache/aurora/scheduler/http/SchedulerzJob.java 
> 2ccc6f367b9715a0abb3e0673069289ae4860087 
>   src/main/java/org/apache/aurora/scheduler/http/ServletModule.java 
> e3ff2571d95effcf72b2047cc5840d56143a180c 
>   src/main/java/org/apache/aurora/scheduler/http/UIRedirectFilter.java 
> b35aad8db006133ba70692e43db1f083d4950914 
>   
> src/main/resources/org/apache/aurora/scheduler/http/assets/datatables/css/jquery.dataTables.css
>  ee6da23c3948a87b0d4df73e82e5c4e2ab2bc803 
>   
> src/main/resources/org/apache/aurora/scheduler/http/assets/datatables/images/back_disabled.png
>  881de7976ff98955e2a5487dca66e618a0655f3d 
>   
> src/main/resources/org/apache/aurora/scheduler/http/assets/datatables/images/back_enabled.png
>  c608682b04a6d9b8002602450c8ef7e80ebba099 
>   
> src/main/resources/org/apache/aurora/scheduler/http/assets/datatables/images/back_enabled_hover.png
>  d300f1064b3beac1d7d5274e294494d3143e53a2 
>   
> src/main/resources/org/apache/aurora/scheduler/http/assets/datatables/images/forward_disabled.png
>  6a6ded7de821619aedc71d1738c0b73463a4452e 
>   
> src/main/resources/org/apache/aurora/scheduler/http/assets/datatables/images/forward_enabled.png
>  a4e6b5384b8454ee7f44a8f7c75b0321b7eeb9b1 
>   
> src/main/resources/org/apache/aurora/scheduler/http/assets/datatables/images/forward_enabled_hover.png
>  fc46c5ebf0524b72a509fe2d7c1bc74995cb8a9d 
>   
> src/main/resources/org/apache/aurora/scheduler/http/assets/datatables/images/sort_asc.png
>  a88d7975fe9017e4e5f2289a94bd1ed66a5f59dc 
>   
> src/main/resources/org/apache/aurora/scheduler/http/assets/datatables/images/sort_asc_disabled.png
>  4e144cf0b1f786a9248a2998311e8109998d8a2d 
>   
> src/main/resources/org/apache/aurora/scheduler/http/assets/datatables/images/sort_both.png
>  18670406bc01ab2721781822dd6478917745ff54 
>   
> src/main/resources/org/apache/aurora/scheduler/http/assets/datatables/images/sort_desc.png
>  def071ed5afd264a036f6d9e75856366fd6ad153 
>   
> src/main/resources/org/apache/aurora/scheduler/http/assets/datatables/images/sort_desc_disabled.png
>  7824973cc60fc1841b16f2cb39323cefcdc3f942 
>   
> src/main/resources/org/apache/aurora/scheduler/http/assets/datatables/js/dataTables.bootstrap.js
>  420e507086f7a2af08f68787f67208546745260d 
>   
> src/main/resources/org/apache/aurora/scheduler/http/assets/datatables/js/dataTables.htmlNumberType.js
>  885ad347f3076dcf75558ef44806dc25a8171884 
>   
> src/main/resources/org/apache/aurora/scheduler/http/assets/datatables/js/dataTables.localstorage.js
>  3c0b39aefbd5a82adee49f2ac7539b6974576b2f 
>   
> src/main/resources/org/apache/aurora/scheduler/http/assets/datatables/js/jquery.dataTables.min.js
>  02694a4a56eb247baa2398d971927dfbd1ac3e60 
>   src/main/resources/org/apache/aurora/scheduler/http/assets/dictionary.js 
> 92045c41706ee9315f3522c3b5bbff7f6e7d2c64 
>   src/main/resources/org/apache/aurora/scheduler/http/assets/util.js 
> 5dd0d17849c7116d70925275b9c6c0715b441f14 
>   src/main/resources/org/apache/aurora/scheduler/http/schedulerzjob.st 
> 28b56671b2e825912a6427e609c2bbe1e7758e26 
>   src/main/resources/org/apache/aurora/scheduler/http/ui/breadcrumb.html 
> PRE-CREATION 
>   src/main/resources/org/apache/aurora/scheduler/http/ui/css/app.css 
> ade850ce624964693e9bd55946464983c4b9f8c2 
>   src/main/resources/org/apache/aurora/scheduler/http/ui/index.html 
> 36225d1e5147e30ba2cb4ddda96dec9f0f2f1dce 
>   src/main/resources/org/apache/aurora/scheduler/http/ui/job.html 
> PRE-CREATION 
>   src/main/resources/org/apache/aurora/scheduler/http/ui/js/app.js 
> db6ea99aeb749fd8674613e3620dc3012872e13c 
>   src/main/resources/org/apache/aurora/scheduler/http/ui/js/controllers.js 
> 7cd534479dd2f17ffd46248ce9af1f8fe89beb97 
>   src/main/resources/org/apache/aurora/scheduler/http/ui/js/directives.js 
> d2b2017a0efc70d425fd6c89ad6caaf46cb8ded5 
>   src/main/resources/org/apache/aurora/scheduler/http/ui/js/filters.js 
> PRE-CREATION 
>   src/main/resources/org/apache/aurora/scheduler/http/ui/js/services.js 
> 8681bbe840b6285b15dd6766016258c2c8115632 
>   src/main/resources/org/apache/aurora/scheduler/http/ui/role.html 
> PRE-CREATION 
>   
> src/main/resources/org/apache/aurora/scheduler/http/ui/schedulingDetail.html 
> PRE-CREATION 
>   src/main/resources/org/apache/aurora/scheduler/http/ui/taskLink.html 
> PRE-CREATION 
>   src/main/resources/org/apache/aurora/scheduler/http/ui/taskSandbox.html 
> PRE-CREATION 
>   src/main/resources/org/apache/aurora/scheduler/http/ui/taskStatus.html 
> PRE-CREATION 
>   src/main/thrift/org/apache/aurora/gen/api.thrift 
> c0618e4edebd6f282698abfd9bdc3c36fff16920 
>   src/test/java/org/apache/aurora/scheduler/app/SchedulerIT.java 
> a4e9464f7d5d3f5a640b62557c3e29f2f1566985 
>   
> src/test/java/org/apache/aurora/scheduler/thrift/SchedulerThriftInterfaceTest.java
>  fae2de11235dd059718e1023fdcfb0e8fc4deadd 
>   
> src/test/java/org/apache/aurora/scheduler/thrift/aop/ServerInfoInterceptorTest.java
>  dd991fb90889c3f221e537a78283eb4a31e5f0dd 
>   src/test/resources/org/apache/aurora/gen/api.thrift.md5 
> 05c6e8a1e2407f2822b3844c555d8995f1cd1d49 
> 
> Diff: https://reviews.apache.org/r/19833/diff/
> 
> 
> Testing
> -------
> 
> ./gradlew clean build run on local laptop.
> 
> 
> Thanks,
> 
> Suman Karumuri
> 
>

Reply via email to