> On Aug. 14, 2015, 3:54 p.m., David McLaughlin wrote:
> > src/main/resources/scheduler/assets/js/controllers.js, line 606
> > <https://reviews.apache.org/r/37365/diff/7/?file=1040096#file1040096line606>
> >
> >     You can just do BaseJobController($scope, ...) now? Here and in other 
> > invocations.
> 
> Joshua Cohen wrote:
>     This is pretty standard mechanism for inheritance in JS. We could remove 
> the .call invocation, but then if something changed in the future that *did* 
> bind to `this` in the parent it wouldn't work properly. I'd prefer to keep it 
> as is.
> 
> David McLaughlin wrote:
>     So maybe I don't understand why you're using classical inheritance here 
> then, over a vanilla function?
> 
> Joshua Cohen wrote:
>     I guess it just made more sense conceptually, but maybe that's because 
> I've been spending way too much time writing java lately ;). Do you think a 
> vanilla function is easier to understand in this context?
> 
> David McLaughlin wrote:
>     Yeah I mean if we keep it this way, it seems to be advocating an approach 
> where any simple function needs to be explicit about what 'this' is when 
> invoked, just in case it's ever used? Does that make sense?
> 
> Joshua Cohen wrote:
>     On further reflection, I *am* using inheritance to expose addColumn and 
> taskIdColumn to the instance controller, so binding to `this` is necessary 
> (see line ~620 in controllers.js). The alternative would be to make them 
> loose functions/variables, which is unappealing since it breaks encapsulation.
> 
> David McLaughlin wrote:
>     On a quick glance it seems like those two functions don't rely on state 
> pulled into the closure. Can't you just move them into services since they 
> are effectively 'static' functions?

Oh! Maybe! I'm not so good at Angular. Maybe the whole bit of shared 
functionality between the two controllers should really be a service!


- Joshua


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


On Aug. 14, 2015, 7:17 p.m., Joshua Cohen wrote:
> 
> -----------------------------------------------------------
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/37365/
> -----------------------------------------------------------
> 
> (Updated Aug. 14, 2015, 7:17 p.m.)
> 
> 
> Review request for Aurora, David McLaughlin and Maxim Khutornenko.
> 
> 
> Repository: aurora
> 
> 
> Description
> -------
> 
> Add a new UI page to show all tasks (active and completed) for a specific 
> instance id.
> 
> N.B.: changes to controllers.js look significant, but it's 90% refactoring so 
> that JobController and JobInstanceController can reuse common code.
> 
> 
> Diffs
> -----
> 
>   src/main/python/apache/aurora/client/base.py 
> dee4c28a26828912bec073c7e5213a03c9703eee 
>   src/main/resources/scheduler/assets/breadcrumb.html 
> 13147935c3c28680d5decd1e03e40d9cf04aaed3 
>   src/main/resources/scheduler/assets/css/app.css 
> a4735efe58a09a46f2b95ebe98f219cb9e7ee27d 
>   src/main/resources/scheduler/assets/instance.html PRE-CREATION 
>   src/main/resources/scheduler/assets/job.html 
> 942635a61132bb9a38a6faf3f8fe046e6bcabd95 
>   src/main/resources/scheduler/assets/js/app.js 
> b66409f628046c67b62c92a75cbeed20c09b4ec1 
>   src/main/resources/scheduler/assets/js/controllers.js 
> 04ea1cb6998af7c238eb2fd1a7d05a9663abc56c 
>   src/main/resources/scheduler/assets/js/services.js 
> a514fa7da3c049e1f5a3e49263a82cf2c28d1b2d 
>   src/main/resources/scheduler/assets/taskInstance.html PRE-CREATION 
>   src/test/python/apache/aurora/client/cli/test_supdate.py 
> 21bea702a6e6fe68652e2195c5d24b56b6477738 
>   src/test/python/apache/aurora/client/test_base.py 
> 1a560088279ac945cce14d02454e50b8483771e4 
> 
> Diff: https://reviews.apache.org/r/37365/diff/
> 
> 
> Testing
> -------
> 
> - ./gradlew jsHint
> - ./pants test src/test/python/apache/aurora/client:base
> - Verified update page url in client was correct in Vagrant.
> - See attached screenshot for an example of what the instance page looks like.
> 
> 
> File Attachments
> ----------------
> 
> Example of instance page in action.
>   
> https://reviews.apache.org/media/uploaded/files/2015/08/11/2ab72f9d-9e60-4b86-b971-b6aba5465def__Screen_Shot_2015-08-11_at_4.53.07_PM.png
> 
> 
> Thanks,
> 
> Joshua Cohen
> 
>

Reply via email to