[GitHub] zeppelin issue #1940: [ZEPPELIN-2008] Introduce Spell
Github user Leemoonsoo commented on the issue: https://github.com/apache/zeppelin/pull/1940 Thanks @1ambda for contributing very interesting new feature. I've play around with it and have couple of feedbacks. ### Couldn't make Spell as display system work > The most important thing is, spell is not only interpreter but also display system. Because it runs on browser. This enable us to combine existing backend interpreters with spell like So i tried following but couldn't make it work. Am i missing something?  ### Configuring Display order of Spell doesn't make sense Enabled spells are displayed in 'Helium' menu like below.  Which is designed to change display order of visualization package when they displayed in chart selector inside of the paragraph. As far as i understand, Spell does not have to be listed in the chart selector and does not have to be listed there and change order. So i think reordering should be only for 'Visualization' type package. I think Spell can create another section just below enabled visualization buttons, to list enabled spells. ### Display keyword of the spell Each spell has keyword to invoke. For example `markdown-spell`'s keyword is `%markdown`, `flowchart-spell`'s keyword is `%flowchart`. However, i couldn't find these keyword anywhere but the source code. So it'll be nice if 'Helium' menu displays keyword of the Spell type packages. ### Example for example spells If there's some informations or example that how to use (syntax, etc) included example Spells, (e.g. in description section or somewhere) it'll be more convenient. --- 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. ---
[GitHub] zeppelin issue #1940: [ZEPPELIN-2008] Introduce Spell
Github user 1ambda commented on the issue:
https://github.com/apache/zeppelin/pull/1940
rebased to resolve conflict with master.
@Leemoonsoo Thanks for detailed review! I updated code according to your
comment.
(1) **Couldn't make Spell as display system work:** Let's handle in
following PRs.
originally, i meant "we can use spell display with backend interpreter **if
we modify some backend code**". Sorry for confusing. I tried to fix it in this
PR, but it's not that easy as i expected since current `InterpreterResult.Type`
is enum, not string. IMO, it can be handled in following PRs. (I modified this
PR description too)
-
https://github.com/apache/zeppelin/blob/master/zeppelin-interpreter/src/main/java/org/apache/zeppelin/interpreter/InterpreterOutput.java#L225
(2) **Configuring Display order of Spell doesn't make sense**: Fixed.
(3, 4) **Display keyword of the spell**, **Example for example spells**
Now `#/helium` page displays `spell.magic`, `spell.usage` which are saved
helium package json.
https://cloud.githubusercontent.com/assets/4968473/22409610/b00f928c-e6cf-11e6-9035-8692cc32e345.png";>
```
{
"type" : "SPELL",
"name" : "translator-spell",
...
"spell": {
"magic": "%translator",
"usage": "%translator - "
}
}
```
---
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.
---
[GitHub] zeppelin issue #1940: [ZEPPELIN-2008] Introduce Spell
Github user Leemoonsoo commented on the issue: https://github.com/apache/zeppelin/pull/1940 Thanks addressing the comments. Tested and it works well as expected. Agree, 1) can be handled in separate issue while this PR already quite big. CI fails on RAT check profile ``` /home/travis/build/apache/zeppelin/zeppelin-interpreter/src/main/java/org/apache/zeppelin/helium/HeliumType.java /home/travis/build/apache/zeppelin/zeppelin-interpreter/src/main/java/org/apache/zeppelin/helium/SpellPackageInfo.java /home/travis/build/apache/zeppelin/zeppelin-interpreter/src/test/java/org/apache/zeppelin/helium/HeliumPackageTest.java ``` Looks like above 3 file need license header. @1ambda could you check? --- 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. ---
[GitHub] zeppelin issue #1940: [ZEPPELIN-2008] Introduce Spell
Github user 1ambda commented on the issue: https://github.com/apache/zeppelin/pull/1940 finally, CI is green. :) --- 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. ---
[GitHub] zeppelin issue #1940: [ZEPPELIN-2008] Introduce Spell
Github user Leemoonsoo commented on the issue: https://github.com/apache/zeppelin/pull/1940 @1ambda Awesome new feature! LGTM --- 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. ---
[GitHub] zeppelin issue #1940: [ZEPPELIN-2008] Introduce Spell
Github user Leemoonsoo commented on the issue: https://github.com/apache/zeppelin/pull/1940 Merge to master if no further comments. --- 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. ---
[GitHub] zeppelin issue #1940: [ZEPPELIN-2008] Introduce Spell
Github user AhyoungRyu commented on the issue: https://github.com/apache/zeppelin/pull/1940 From now, `SPELL` type pkg info fetched from [Helium registry](https://s3.amazonaws.com/helium-package/helium.json) is also shown in GUI :) https://cloud.githubusercontent.com/assets/10060731/22501386/e56b8336-e8ab-11e6-94ee-e0bed54fe2a1.png";> --- 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. ---
