Skip to content

[ZEPPELIN-1026] set syntax highlight based on default bound interpreter#1415

Closed
minahlee wants to merge 10 commits intoapache:masterfrom
minahlee:ZEPPELIN-1026
Closed

[ZEPPELIN-1026] set syntax highlight based on default bound interpreter#1415
minahlee wants to merge 10 commits intoapache:masterfrom
minahlee:ZEPPELIN-1026

Conversation

@minahlee
Copy link
Copy Markdown
Member

@minahlee minahlee commented Sep 8, 2016

What is this PR for?

This is complete work of #1148. Comments and tasks on #1148 has been handled in this PR.

  • Add syntax language information in interpreter-setting.json
  • When user type %replName in paragraph, back-end check if the interpreter name with replName exists, and return language information to front-end if it does
  • If user doesn't specify %replName, default interpreter's language will be used
  • Using alias name for paragraph syntax highlight

What type of PR is it?

[Bug Fix | Improvement]

What is the Jira issue?

ZEPPELIN-1026

How should this be tested?

  1. Create new note and make markdown interpreter to be default.
  2. See if markdown syntax is applied.

Screenshots (if appropriate)

Case 1. When the default interpreter set to python interpreter.

Before
Has scala as syntax highlight language when %python is not set.
screen shot 2016-07-07 at 10 46 20 pm

After
Has python as syntax highlight language even when %python is not set.
screen shot 2016-07-07 at 10 44 39 pm

Case 2. When use alias name as repl name.

Before
screen shot 2016-09-08 at 4 22 39 pm

After
screen shot 2016-09-08 at 4 34 57 pm

Further possible improvements

There are still several cases that Zeppelin doesn't handle syntax highlight well. These can be handled with another jira ticket/PR.

  1. When default bound interpreter changes, syntax highlight is not changed accordingly
  2. When copy/paste code, syntax highlight won't be applied properly since Zeppelin only checks changes when cursor is in first line.

Questions:

  • Does the licenses files need update? no
  • Is there breaking changes for older versions? no
  • Does this needs documentation? yes(for creating new interpreter)

@minahlee minahlee changed the title [ZEPPELIN-1026] set syntax highlight based on default bound interpreter [WIP][ZEPPELIN-1026] set syntax highlight based on default bound interpreter Sep 9, 2016
@minahlee minahlee changed the title [WIP][ZEPPELIN-1026] set syntax highlight based on default bound interpreter [ZEPPELIN-1026] set syntax highlight based on default bound interpreter Sep 12, 2016
@minahlee
Copy link
Copy Markdown
Member Author

minahlee commented Sep 12, 2016

CI is green, ready for review.

bzz
bzz reviewed Sep 13, 2016
View reviewed changes
docs/development/writingzeppelininterpreter.md
"description": "Property 2 description"
}, ...
},
"editor": {
Copy link
Copy Markdown
Member

@bzz bzz Sep 13, 2016

Choose a reason for hiding this comment

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

Quick question - it's a bit not clear if this section is mandatory and if not, what is going to be the default one? May be it could be described in docs below.
Asking as there are i.e PRs that implement a new interpreters, before this section was introduced.

Copy link
Copy Markdown
Member Author

@minahlee minahlee Sep 13, 2016 •
edited
Loading

Choose a reason for hiding this comment

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

Good point! If the language is not specified plain text mode will be used for syntax highlight, which will highlight nothing. I added description in fd7896e. Thanks for review :)

@bzz
Copy link
Copy Markdown
Member

bzz commented Sep 13, 2016

Looks great to me.

Leemoonsoo
Leemoonsoo reviewed Sep 13, 2016
View reviewed changes
zeppelin-zengine/src/main/java/org/apache/zeppelin/notebook/Note.java Outdated
return factory.getInterpreter(getId(), name);
}

public Map<String, Object> getEditorSetting(String replName) {
Copy link
Copy Markdown
Member

@Leemoonsoo Leemoonsoo Sep 13, 2016

Choose a reason for hiding this comment

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

I think this method is more make sense to be placed in InterpreterFactory, while editor setting is related interpreter and interpreter setting, not each note. What do you think?

Copy link
Copy Markdown
Member Author

@minahlee minahlee Sep 19, 2016 •
edited
Loading

Choose a reason for hiding this comment

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

@Leemoonsoo It totally makes sense, I made change in e810da4. Please review.

@felixcheung felixcheung mentioned this pull request Sep 17, 2016
Closed
@minahlee minahlee force-pushed the ZEPPELIN-1026 branch 4 times, most recently from 69fc151 to e810da4 Compare September 21, 2016 08:39
@Leemoonsoo
Copy link
Copy Markdown
Member

Leemoonsoo commented Sep 21, 2016

@minahlee Great work! LGTM

@minahlee minahlee force-pushed the ZEPPELIN-1026 branch 3 times, most recently from ed2f473 to aa5eb2f Compare September 22, 2016 09:53
@minahlee
Copy link
Copy Markdown
Member Author

minahlee commented Sep 22, 2016

Merge if there is no more discussion

@asfgit asfgit closed this in a3ca800 Sep 23, 2016
@minahlee minahlee deleted the ZEPPELIN-1026 branch September 24, 2016 19:05
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Reviewers

No reviews

Assignees

No one assigned

Labels

None yet

Projects

None yet

Milestone

No milestone

Development

Successfully merging this pull request may close these issues.

3 participants

@minahlee @bzz @Leemoonsoo