Github user zjffdu commented on the issue:
https://github.com/apache/zeppelin/pull/2272
Thanks @jongyoul Will merge it 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
Github user jongyoul commented on the issue:
https://github.com/apache/zeppelin/pull/2272
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 zjffdu commented on the issue:
https://github.com/apache/zeppelin/pull/2272
@jongyoul @felixcheung Comments are address, please help 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
Github user zjffdu commented on the issue:
https://github.com/apache/zeppelin/pull/2272
Thanks @felixcheung @jongyoul for the review. Comments are addressed.
---
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
Github user zjffdu commented on the issue:
https://github.com/apache/zeppelin/pull/2272
I am fine to add interface for it. But only `JsonSerializable` can be
added, because `fromJson` is static method. It is some kind of code
convention, should be easy to read and understand.
---
Github user jongyoul commented on the issue:
https://github.com/apache/zeppelin/pull/2272
I saw it above. It doesn't reduce the code size but I think it will help
another contributors understand code well. How do you think of it?
---
If your project is set up for it, you can reply
Github user zjffdu commented on the issue:
https://github.com/apache/zeppelin/pull/2272
@jongyoul Thanks for the review. Copy my comment above.
```
toJson and fromJson is used in both zeppelin-interpreter and
zeppelin-zengine. I didn't find a proper place to put the common
Github user jongyoul commented on the issue:
https://github.com/apache/zeppelin/pull/2272
LGTM. For the next step, how about introducing `JsonSerializable` and
`JsonDeserializable` as interfaces?
---
If your project is set up for it, you can reply to this email and have your
reply
Github user zjffdu commented on the issue:
https://github.com/apache/zeppelin/pull/2272
@felixcheung @Leemoonsoo Any more comments on this ? This PR also address
the issue in ZEPPELIN-2652.
Failed test is flaky test. I will address it in #2422
```
Failed tests:
Github user zjffdu commented on the issue:
https://github.com/apache/zeppelin/pull/2272
@felixcheung toJson and fromJson is used in both zeppelin-interpreter and
zeppelin-zengine. I didn't find a proper place to put the common interface. And
actually the interface doesn't not
Github user felixcheung commented on the issue:
https://github.com/apache/zeppelin/pull/2272
this seems like a good change. perhaps toJSON etc should be in a common
base class to avoid duplications?
---
If your project is set up for it, you can reply to this email and have your
11 matches
Mail list logo
