Skip to content

ZEPPELIN-219: Paragaph mode auto-detection - initial impl - #206

Closed
tzolov wants to merge 4 commits into
apache:masterfrom
tzolov:ZEPPELIN-219
Closed

ZEPPELIN-219: Paragaph mode auto-detection - initial impl#206
tzolov wants to merge 4 commits into
apache:masterfrom
tzolov:ZEPPELIN-219

Conversation

@tzolov

Copy link
Copy Markdown
Contributor

Here are the mode autodetection rules (and optimizations):

  • Evaluates the editor mode on each aceChange event.
  • (Optimization 1) Evaluates the mode only if the cursor is within the first 30 characters of the buffer.
  • (Optimization 2) Sets the mode (e.g. session.setMode(new mode) ) only if the new mode differs from the previous one.

Also the mode tag detection structure is refactored to bring together the interpreter tag test and the mode reference. The convention looks like this:

vareditorModes=[[<YourMode1RegExpressionTest>, <ACEMode1>],
......
[<YourModeNRegExpressionTest>, <ACEModeN>]
];

For example:

vareditorModes=[[/^%spark/,'ace/mode/scala'],[/^%(\w*\.)?\wql/,'ace/mode/sql'],[/^%md/,'ace/mode/markdown'],[/^%sh/,'ace/mode/sh']];

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

reuse the text gotten in aceChanged? how bad is it to get this again?

Copy link
Copy Markdown
ContributorAuthor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I will reuse the text value. Don't know what is the performance implication of calling it again.

@tzolov

Copy link
Copy Markdown
ContributorAuthor

Note: apparently i didn't commit all changes with the latest commit (40b9a33). Will redo it again.

@tzolov
tzolovforce-pushed the ZEPPELIN-219 branch 3 times, most recently from 7fe3b19 to ff896a4CompareAugust 13, 2015 20:00
@djoelz

Copy link
Copy Markdown

I would love to see some unit tests for this.

@tzolov

Copy link
Copy Markdown
ContributorAuthor

@djoelz, this change affects paragraph.controller.js. I am not sure i've seen units tests for this or the other js controllers? But i might have missed something.

@Leemoonsoo is there some samples for testing such behavior?

@Leemoonsoo

Copy link
Copy Markdown
Member

Two possible places for test. test controllers and directives or test using selenium

@tzolov
tzolovforce-pushed the ZEPPELIN-219 branch 4 times, most recently from 7a779b0 to c0cdbc2CompareAugust 24, 2015 07:10
@tzolov

Copy link
Copy Markdown
ContributorAuthor

@Leemoonsoo thanks for the references. If not mistaken the test controllers and directives doesn't contain any real controller tests at the moment. As i've mentioned earlier i'm not much of js/angular authority and would prefer to learn from someone else's experience and examples. @djoelz do you feel like you can help here?
Otherwise we can review and merge this and postpone the tests for after someone provide an example such?

@corneadoug

Copy link
Copy Markdown
Contributor

Maybe I can help with those tests, I guess we need:

  • Some tests for the setMode function
  • A test scenario for Optimization 1(typing 30 characters and having setMode called)

@tzolov

Copy link
Copy Markdown
ContributorAuthor

@corneadoug, your help will be well appreciated! The suggested testset sounds good and covering the functionality.

@djoelz

Copy link
Copy Markdown

@tzolov I am also new to the angular/js world but if @corneadoug can provide a starting point I can help with the more tests.

@tzolov
tzolovforce-pushed the ZEPPELIN-219 branch 2 times, most recently from b24f0d5 to a507e3eCompareAugust 26, 2015 07:03
@corneadoug

Copy link
Copy Markdown
Contributor

Hi, Sorry guys I'm taking a bit of time on this one.
I actually started by understanding the code to know what test I could make, and ended up spending time optimizing the loop and conditions first :P
One problem though is that I can't find @tzolov repository in the github interface when I try to make a pull request :/

@bzz

bzz commented Aug 27, 2015

Copy link
Copy Markdown
Member

@corneadoug not sure but think that to be able to see it in "create new PR" ui you need to make sure that the branch you are using is actually derived from https://github.com/tzolov/incubator-zeppelin/tree/ZEPPELIN-219 (either checkouted from ZEPPELIN-219 branch of the clean clone of tzolov/incubator-zeppelin or from the new 'remote' pointing here, inside your existing repo)

You can verify it with gitk command (which comes with brew install git)

@corneadoug

Copy link
Copy Markdown
Contributor

I created a remote from his repo, then used his branch as a base.
The weird part being that I have the choice of everybody except him

@corneadoug

Copy link
Copy Markdown
Contributor

@tzolov can you merge this into your branch then? https://github.com/corneadoug/incubator-zeppelin/tree/improve/ZEPPELIN-219

@tzolov

Copy link
Copy Markdown
ContributorAuthor

@corneadoug, thanks for the improvements.
just merged the corneadoug/improve/ZEPPELIN-219 branch

@tzolov
tzolovforce-pushed the ZEPPELIN-219 branch 2 times, most recently from fe37ef9 to 518f009CompareSeptember 2, 2015 08:24
@tzolov

Copy link
Copy Markdown
ContributorAuthor

@corneadoug, have you had chance on writing an unit test(s) for this PR?
If you have not time and there are no other objections may i suggest to have this improvement merged? It is been hanging around for some time now and there few other tickets depending on it as well.

@corneadoug

Copy link
Copy Markdown
Contributor

@tzolov I started doing it, if possible give me a day to finish it up

@tzolov

Copy link
Copy Markdown
ContributorAuthor

@corneadoug, glad to hear you are working on this. Take your time. Your help is highly appreciated and will help me/us learn how to write such tests!

@corneadoug

Copy link
Copy Markdown
Contributor

@tzolov let's rebase once #250 is merged, so that I can do a PR without conflicts

@tzolov

Copy link
Copy Markdown
ContributorAuthor

@corneadoug it is done. just rebased (after ZEPPELIN-228) and pushed.

@corneadoug

Copy link
Copy Markdown
Contributor

Just spent a lot of time on this.
I think we will have to create an issue, and do the tests later.
With Current version of ui-ace that we use, we can't test it because of a bug (angular-ui/ui-ace#73)
We would need to Bump to newer version (0.2.x), but those versions have breaking changes.
I pretty much figured out what we need to change, but can't guarantee that everything would be working (it would take lot of time to manually test + write tests). which would delay more this PR.
So the best would be to do the test later.

@tzolov

Copy link
Copy Markdown
ContributorAuthor

Thanks for the update @corneadoug. I've faced the ace upgrade issue while preparing the (ZEPPELIN-216). When i tried to upgrade ace to 1.2.0 the existing ui-ace version complained. I've managed to increase ui-ace but had to force some dependency resolutions and was not confident it will not cause side effects. So i upgraded ace to safest ace 1.1.9 version instead. But if you manage to upgrade ui-ace (very outdated it seems) it will allow us upgrade ace-build as well.
Also i would still love to learn how to write angular controller unit tests.

@Leemoonsoo , @felixcheung it seems that this PR (e.g. ZEPPELIN-219) is ready to merge.
The tests will come after the ui-ace upgrade.

@Madhuka

Copy link
Copy Markdown
Contributor

@tzolov : You can find few test in PR #262,
+1 for the merge (I too can support on writing test)

@Leemoonsoo

Copy link
Copy Markdown
Member

Looks good to me!

@asfgitasfgit closed this in eebae39Sep 9, 2015
Leemoonsoo pushed a commit to Leemoonsoo/zeppelin that referenced this pull request Sep 17, 2015
Here are the mode autodetection rules (and optimizations):
- Evaluates the editor mode on each `aceChange` event.
- (Optimization 1) Evaluates the mode only if the cursor is within the first 30 characters of the buffer.
- (Optimization 2) Sets the mode (e.g. `session.setMode(new mode)` ) only if the new mode differs from the previous one.
Also the mode tag detection structure is refactored to bring together the interpreter tag test and the mode reference. The convention looks like this:
```javascript
var editorModes = [
[<Your Mode 1 Reg Expression Test>, <ACE Mode 1>],
......
[<Your Mode N Reg Expression Test>, <ACE Mode N>]
];
```
For example:
```javascript
var editorModes = [
[/^%spark/, 'ace/mode/scala'],
[/^%(\w*\.)?\wql/, 'ace/mode/sql'],
[/^%md/, 'ace/mode/markdown'],
[/^%sh/, 'ace/mode/sh']
];
```
Author: Damien Corneau <corneadoug@gmail.com>
Author: tzolov <christian.tzolov@gmail.com>
Closesapache#206 from tzolov/ZEPPELIN-219 and squashes the following commits:
fda3f5e [Damien Corneau] Improve loop performances
153f327 [Damien Corneau] Change setParagraphMode function
ea76881 [tzolov] ZEPPELIN-219: Add paragraph text as setPargraphMode() argument
aab3b7c [tzolov] ZEPPELIN-219: Paragaph mode auto-detection - initial impl
(cherry picked from commit eebae39)
Signed-off-by: Lee moon soo <moon@apache.org>
mapr-devops pushed a commit to mapr/zeppelin that referenced this pull request May 8, 2025
Sign up for freeto join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

8 participants

@tzolov@djoelz@Leemoonsoo@corneadoug@bzz@Madhuka@martin-g@felixcheung