Github user Leemoonsoo commented on the pull request:

    https://github.com/apache/incubator-zeppelin/pull/678#issuecomment-177319760
  
    @doanduyhai Thanks for great work.
    I have look through this PR and while this PR covers wide range of 
improvement/new features, i'd suggest split it into multiple smaller PR.
    
    > #### 1) AngularJS Utility Function,
    
    This is the first time exposing Zeppelin provided javascript function to 
user. How about we make them little bit different from other functions defined 
in the scope. For example instead of
    
    ```
    $scope.runParagraph(..)
    $scope.pushToServer(...)
    ```
    
    collect all exposed function into `z`
    
    ```
    $scope.z.runParagraph(...)
    $scope.z.pushToServer(...)
    ```
    
    I this way, both user/developer can explicitly recognize which is exposed 
function to user. What do you think?
    
    
    <br />
    
    > * `pushToServer('varName', value, parameters)`
    
    Once angularObject is created (binded), no matter who (front-end / 
back-end) creates, every change of binded scope variable automatically pushed 
to server.
    So this function name may confuse a bit. How about different name, such as 
`bindAngularObject()` ?
    
    And this function give user access to any scope of AngularObjectRegistry of 
any Interpreter process.
    Is there any special reason? otherwise can we restrict parameters from
    
    ```
    parameters = {
         interpreter: 'spark',
         interpreters: ['md', 'sh'],
         paragraph: '20160126-153136_1060166247',
         paragraphs: ['20160126-171914_843040190', 
'20160126-181556_1915782845'],
         scope: 'note',
         runParagraphs: false    
    }
    ```
    
    to
    
    ```
    parameters = {
        scope : 'note'
    }
    ```
    
    So user's javascript code only able to access current paragraph or notebook 
scope of AngularObject in the current interpreter process.
    
    <br />
    
    > 3) Technical Impl
    Regarding initialize AngularObjectRegistry on Interpreter creation using 
`angularRegistryPush()` looks good.
    
    And do you mind making separate PR for change regarding dynamic form?
    That i'm not quite sure the benefit of reading angularObject in dynamic 
form compare to the complexity it brings. I mean complexity of the concept and 
expected behavior that user need to understand.
    



---
If your project is set up for it, you can reply to this email and have your
reply appear on GitHub as well. If your project does not have this feature
enabled and wishes so, or if the feature is enabled but not working, please
contact infrastructure at infrastruct...@apache.org or file a JIRA ticket
with INFRA.
---

Reply via email to