Github user 1ambda commented on the issue:
https://github.com/apache/zeppelin/pull/2268
merge if no more discussion.
---
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
Github user tinkoff-dwh commented on the issue:
https://github.com/apache/zeppelin/pull/2268
@1ambda
done
---
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
Github user 1ambda commented on the issue:
https://github.com/apache/zeppelin/pull/2268
@tinkoff-dwh Please resolve conflict.
Usually, we merge after 1 day when someone left LGTM, but this time,
conflict occurred before 1 day.
---
If your project is set up for it, you
Github user 1ambda commented on the issue:
https://github.com/apache/zeppelin/pull/2268
LGTM, merge if no more discussion.
---
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
Github user tinkoff-dwh commented on the issue:
https://github.com/apache/zeppelin/pull/2268
1,2 done
@1ambda thx for review
---
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
Github user 1ambda commented on the issue:
https://github.com/apache/zeppelin/pull/2268
1. Please resolve the conflict.
2. Left a minor comment please check that.
3. Tested locally and works well with 6 newly added types.
4. Backward compatibility will be handed by the
Github user 1ambda commented on the issue:
https://github.com/apache/zeppelin/pull/2268
I am not a big fan of this approach. But reviewers discussed and I
partially agree in terms of complexity.
I will check few things since it was quite old when I last reviewed this,
Github user tinkoff-dwh commented on the issue:
https://github.com/apache/zeppelin/pull/2268
@Leemoonsoo @1ambda
maybe merge? 2 month :)
---
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
Github user tinkoff-dwh commented on the issue:
https://github.com/apache/zeppelin/pull/2268
Ready to review :)
---
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
Github user tinkoff-dwh commented on the issue:
https://github.com/apache/zeppelin/pull/2268
Ready to review
---
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
Github user tinkoff-dwh commented on the issue:
https://github.com/apache/zeppelin/pull/2268
@Leemoonsoo
?
---
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
Github user tinkoff-dwh commented on the issue:
https://github.com/apache/zeppelin/pull/2268
@Leemoonsoo
done
---
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
Github user 1ambda commented on the issue:
https://github.com/apache/zeppelin/pull/2268
IMO, It's ok if we are not going to add other types like `JSON` which can
be `type` and used for other widgets (e.g `textarea`, ...)
---
If your project is set up for it, you can reply to this
Github user tinkoff-dwh commented on the issue:
https://github.com/apache/zeppelin/pull/2268
looks good. initially, the idea with the type well, as the type indicated
the type of the stored value (widget is pointing to), but **password** all
broke)
@1ambda
what do you
Github user Leemoonsoo commented on the issue:
https://github.com/apache/zeppelin/pull/2268
If i list all combinations of widget-type in this PR,
| widget | type |
| - | - |
| input | **string** |
| input | **number** |
|
Github user tinkoff-dwh commented on the issue:
https://github.com/apache/zeppelin/pull/2268
@Leemoonsoo
first three comments
...
I think it's not complicated. IMO, it's the part we should design carefully
since it's hard to change (backward compatibility)
...
Github user Leemoonsoo commented on the issue:
https://github.com/apache/zeppelin/pull/2268
> the separation discussed above in the comments
maybe i missed, @tinkoff-dwh could you point out the comment?
---
If your project is set up for it, you can reply to this email and
Github user 1ambda commented on the issue:
https://github.com/apache/zeppelin/pull/2268
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
Github user tinkoff-dwh commented on the issue:
https://github.com/apache/zeppelin/pull/2268
ready to review
---
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
Github user tinkoff-dwh commented on the issue:
https://github.com/apache/zeppelin/pull/2268
CI red https://travis-ci.org/tinkoff-dwh/zeppelin/builds/231079773
another tests
SecurityRestApiTest.testGetUserList:69 » JsonSyntax
java.lang.IllegalStateExce...
Github user tinkoff-dwh commented on the issue:
https://github.com/apache/zeppelin/pull/2268
@Leemoonsoo
the separation discussed above in the 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
Github user Leemoonsoo commented on the issue:
https://github.com/apache/zeppelin/pull/2268
This PR adds two new field 'widget' and 'type' to each properties.
'widget' : input, checkbox, textarea, password
'type' : number, string, boolean, url, password
How about
Github user 1ambda commented on the issue:
https://github.com/apache/zeppelin/pull/2268
As far as I Know, Zeppelin supports backward compatibility even for
interpreter settings.
- I think we need some migration code w/ documentation
---
If your project is set up for it, you
Github user tinkoff-dwh commented on the issue:
https://github.com/apache/zeppelin/pull/2268
@1ambda
i wrote about migration in upgrade.md. I'll see Notebook.java if the
documentation is not enough
---
If your project is set up for it, you can reply to this email and have your
Github user 1ambda commented on the issue:
https://github.com/apache/zeppelin/pull/2268
Sorry for the late review.
seems that it doesn't work with existing settings. Even can't start
Zeppelin instance.
```
Caused by: java.lang.ClassCastException:
Github user 1ambda commented on the issue:
https://github.com/apache/zeppelin/pull/2268
Let me review and comment soon for this awesome improvement.
---
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
Github user tinkoff-dwh commented on the issue:
https://github.com/apache/zeppelin/pull/2268
Ready to review
---
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
Github user tinkoff-dwh commented on the issue:
https://github.com/apache/zeppelin/pull/2268
CI red (checkstyle errors in all modules)
---
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
Github user 1ambda commented on the issue:
https://github.com/apache/zeppelin/pull/2268
One example of separating widget with value types.
```
//
https://github.com/apache/zeppelin/blob/master/zeppelin-web/src/app/tabledata/advanced-transformation-util.test.js#L19-L25
Github user tinkoff-dwh commented on the issue:
https://github.com/apache/zeppelin/pull/2268
@1ambda
I think it's too complicate code, for example generate list (combobox) of
widgets. How will the selection of the type of the password, second combobox?.
I see no reason to
Github user 1ambda commented on the issue:
https://github.com/apache/zeppelin/pull/2268
1. number, string and url can be generalized as `text`. For url, we can use
`ng-html` with angular's `$sse` service to draw `..` element
2. if `text` widget means `textarea` DOM, can we change
31 matches
Mail list logo
