You are viewing a plain text version of this content. The canonical link for it is here.

Posted to dev@zeppelin.apache.org by malayhm <gi...@git.apache.org> on 2017/08/16 12:51:24 UTC

[GitHub] zeppelin pull request #2542: ZEPPELIN-277 Add Tab as Autocomplete for Notebo...

GitHub user malayhm opened a pull request:

    https://github.com/apache/zeppelin/pull/2542

    ZEPPELIN-277 Add Tab as Autocomplete for Notebook non-md paragraphs

    ### What is this PR for?
    This PR will add tab as auto complete invoker if paragraph is non-md and user has not pressed the tab as a first character in the line
    
    ### What type of PR is it?
    [Improvement]
    
    ### What is the Jira issue?
    * https://issues.apache.org/jira/browse/ZEPPELIN-277
    
    ### How should this be tested?
    
    - Build: mvn clean package -Denforcer.skip -DskipTests -Drat.skip
    - Open a paragraph
    - Press tab with following options: first character, after space
    
    ### Questions:
    * Does the licenses files need an update? no
    * Is there breaking changes for older versions? no
    * Does this needs documentation? no


You can merge this pull request into a Git repository by running:

    $ git pull https://github.com/malayhm/zeppelin ZEPPELIN-277

Alternatively you can review and apply these changes as the patch at:

    https://github.com/apache/zeppelin/pull/2542.patch

To close this pull request, make a commit to your master/trunk branch
with (at least) the following in the commit message:

    This closes #2542
    
----
commit 655ba8881ee329d988bf099c920f877fb6a29458
Author: Malay Majithia <ma...@gmail.com>
Date:   2017-08-16T12:46:47Z

    ZEPPELIN-277 Add Tab as Autocomplete for Notebook non-md paragraphs

----


---
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 infrastructure@apache.org or file a JIRA ticket
with INFRA.
---

[GitHub] zeppelin issue #2542: ZEPPELIN-277 Add Tab as Autocomplete for Notebook non-...

Posted by zjffdu <gi...@git.apache.org>.

Github user zjffdu commented on the issue:

    https://github.com/apache/zeppelin/pull/2542
  
    @malayhm Could you try to return these failed CI build ? https://travis-ci.org/malayhm/zeppelin/builds/288324042


---

[GitHub] zeppelin issue #2542: ZEPPELIN-277 Add Tab as Autocomplete for Notebook non-...

Posted by malayhm <gi...@git.apache.org>.

Github user malayhm commented on the issue:

    https://github.com/apache/zeppelin/pull/2542
  
    @Leemoonsoo one question, why did we not add R to have auto complete on tab?


---

[GitHub] zeppelin issue #2542: ZEPPELIN-277 Add Tab as Autocomplete for Notebook non-...

Posted by zjffdu <gi...@git.apache.org>.

Github user zjffdu commented on the issue:

    https://github.com/apache/zeppelin/pull/2542
  
    ping @malayhm 


---

[GitHub] zeppelin issue #2542: ZEPPELIN-277 Add Tab as Autocomplete for Notebook non-...

Posted by Leemoonsoo <gi...@git.apache.org>.

Github user Leemoonsoo commented on the issue:

    https://github.com/apache/zeppelin/pull/2542
  
    Thanks @malayhm !
    
    LGTM and merge to master if no further comment!


---

[GitHub] zeppelin issue #2542: ZEPPELIN-277 Add Tab as Autocomplete for Notebook non-...

Posted by malayhm <gi...@git.apache.org>.

Github user malayhm commented on the issue:

    https://github.com/apache/zeppelin/pull/2542
  
    @Leemoonsoo Fixed lint error.


---

[GitHub] zeppelin issue #2542: ZEPPELIN-277 Add Tab as Autocomplete for Notebook non-...

Posted by malayhm <gi...@git.apache.org>.

Github user malayhm commented on the issue:

    https://github.com/apache/zeppelin/pull/2542
  
    @Leemoonsoo Added the improvement as per point 1 mentioned above. It will keep adding tab if all the previous characters are a tab in the same line.


---

[GitHub] zeppelin issue #2542: ZEPPELIN-277 Add Tab as Autocomplete for Notebook non-...

Posted by Leemoonsoo <gi...@git.apache.org>.

Github user Leemoonsoo commented on the issue:

    https://github.com/apache/zeppelin/pull/2542
  
    I tested this branch + malayhm#1 + master. Like @zjffdu mentioned master branch doesn't have interpreter setting problem, so it works well without ZEPPELIN-2907.
    
    @malayhm if you review and merge malayhm#1, i think we can merge this pull request.


---

[GitHub] zeppelin issue #2542: ZEPPELIN-277 Add Tab as Autocomplete for Notebook non-...

Posted by zjffdu <gi...@git.apache.org>.

Github user zjffdu commented on the issue:

    https://github.com/apache/zeppelin/pull/2542
  
    > This makes interpreter impossible to add / update "editor" configuration without user recreate interpreterSetting
    
    @Leemoonsoo ZEPPELIN-2907 is not resolved, but the issue you concerned above is fixed. As in the interpreter component refactoring, I would always overwrite editor by using the editor info from  InterpreterSettingTemplate (interpreter-setting.json) (https://github.com/apache/zeppelin/blob/master/zeppelin-zengine/src/main/java/org/apache/zeppelin/interpreter/InterpreterSettingManager.java#L199). And I believe the editor info is immutable, so it should be safe to overwrite it. That means we could always add new field in editor of interpreter-setting.json


---

[GitHub] zeppelin issue #2542: ZEPPELIN-277 Add Tab as Autocomplete for Notebook non-...

Posted by malayhm <gi...@git.apache.org>.

Github user malayhm commented on the issue:

    https://github.com/apache/zeppelin/pull/2542
  
    @Leemoonsoo @zjffdu I have merged the sub level pull request to this PR, we can merge this to master.


---

[GitHub] zeppelin issue #2542: ZEPPELIN-277 Add Tab as Autocomplete for Notebook non-...

Posted by malayhm <gi...@git.apache.org>.

Github user malayhm commented on the issue:

    https://github.com/apache/zeppelin/pull/2542
  
    Sure @Leemoonsoo 


---

[GitHub] zeppelin issue #2542: ZEPPELIN-277 Add Tab as Autocomplete for Notebook non-...

Posted by Leemoonsoo <gi...@git.apache.org>.

Github user Leemoonsoo commented on the issue:

    https://github.com/apache/zeppelin/pull/2542
  
    @malayhm Let me try summarize in this way
    
    a. Merge this PR -> Tab completion works but target interpreter to apply tab completion is hardcoded in the front-end.
    b. Merge this PR + https://github.com/malayhm/zeppelin/pull/1  -> Remove hardcoded part from front-end and let each interpreter choose completion key. But it does not applied to existing interpreter setting  [ZEPPELIN-2907](https://issues.apache.org/jira/browse/ZEPPELIN-2907).
    c. Merge this PR + https://github.com/malayhm/zeppelin/pull/1  + [ZEPPELIN-2907](https://issues.apache.org/jira/browse/ZEPPELIN-2907) -> Everything is happy
    
    
    a) brings something on development of interpreter (i.e. we should document where interpreter developer can find and change completion key from the front-end code).
    And b) brings something on using zeppelin (i.e. we should document the behavior that existing interpreter setting does not have tab completion key support).
    
    If we agree that c) is something we want to deliver to our users, which part do you want to cover in this PR @malayhm ?
    
    And according to comment from @zjffdu
    
    > @Leemoonsoo I hit the interpreter.json upgrade issue when doing interpreter component refactoring, and I think I already fixed it. https://github.com/apache/zeppelin/blob/master/zeppelin-zengine/src/main/java/org/apache/zeppelin/interpreter/InterpreterSettingManager.java#L199
    
    issue  [ZEPPELIN-2907](https://issues.apache.org/jira/browse/ZEPPELIN-2907) is already solved?


---

[GitHub] zeppelin issue #2542: ZEPPELIN-277 Add Tab as Autocomplete for Notebook non-...

Posted by malayhm <gi...@git.apache.org>.

Github user malayhm commented on the issue:

    https://github.com/apache/zeppelin/pull/2542
  
    @Leemoonsoo : I have changed
    
    `let isAllTabs = currentLine.split('').every(function(char) { return (char === '\t' || char === ' ') })`
    to 
    `let isAllTabs = currentLine.substring(0, iCursor.column - 1).split('').every(function(char) { return (char === '\t' || char === ' ') })`


---

[GitHub] zeppelin issue #2542: ZEPPELIN-277 Add Tab as Autocomplete for Notebook non-...

Posted by malayhm <gi...@git.apache.org>.

Github user malayhm commented on the issue:

    https://github.com/apache/zeppelin/pull/2542
  
    @Leemoonsoo @zjffdu I tried merging the latest master with a rerun of the CI job.
    
    Unfortunately, it didn't help. Could you suggest the next step?


---