Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

[ZEPPELIN-1399] Refactor automatic interpreter name insert #1388

Closed
wants to merge 3 commits into from

Conversation

minahlee
Copy link
Member

@minahlee minahlee commented Aug 30, 2016

What is this PR for?

Refine automatic interpreter name insert. See below jira issue for detailed description

What type of PR is it?

Improvement | Refactoring

What is the Jira issue?

ZEPPELIN-1399

Screenshots (if appropriate)

  • Do not insert interpreter name if previous interpreter name is not specified
    Before
    aug-30-2016 17-28-06
    After
    aug-30-2016 17-24-23
  • Insert interpreter name of previous paragraph without running
    Before
    aug-30-2016 17-33-25
    After
    aug-30-2016 17-24-32

Questions:

  • Does the licenses files need update? no
  • Is there breaking changes for older versions? no
  • Does this needs documentation? no

@Leemoonsoo
Copy link
Member

Tested and LGTM.
Thanks @minahlee for the great improvement.

@corneadoug
Copy link
Contributor

@minahlee For the first case, it is actually the default interpreter.
For example, In Master when you create a new Note you will have '%spark' in the paragraph.
In this Branch, it will stay empty.

Another example would be using: sc.version in a paragrah without %spark

@minahlee
Copy link
Member Author

minahlee commented Sep 2, 2016

@corneadoug actually I made it empty intentionally. please check out the discussion here

@corneadoug
Copy link
Contributor

@minahlee Making a new line is a good improvement because some interpreters do not handle same line typing.

For removing the default interpreter, I don't have a strong opinion.
If we plan on having the backend provide the interpreter information in the paragraph and always show it outside of the text editor, we would remove this feature anyway.

However it could feel weird to have Release 0.6.1 have the default interpreter shown, 0.6.2 not showing it, then 0.6.3 or 0.7.0 have Interpreter always shown again but this time as part of the paragraph (not in text editor)

@minahlee
Copy link
Member Author

minahlee commented Sep 2, 2016

I totally agree that we shouldn't make ux change often. But showing default interpreter in text is implemented only in master branch but not in previous release so I think it won't bring much of confusion.

@minahlee
Copy link
Member Author

minahlee commented Sep 2, 2016

Reverted #1248 during the rebase since this PR already has implemented same functionality in this line.

@minahlee minahlee force-pushed the ZEPPELIN-1399 branch 2 times, most recently from 954127f to 5313f13 Compare September 5, 2016 09:34
@minahlee
Copy link
Member Author

minahlee commented Sep 7, 2016

CI is green. Merge if there is no more discussuion

@asfgit asfgit closed this in 3db819a Sep 7, 2016
@minahlee minahlee deleted the ZEPPELIN-1399 branch November 2, 2016 04:59
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
3 participants