[GitHub] incubator-predictionio pull request #425: [PIO-110] Refactoring
Github user asfgit closed the pull request at: https://github.com/apache/incubator-predictionio/pull/425 --- 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] incubator-predictionio pull request #425: [PIO-110] Refactoring
Github user shimamoto commented on a diff in the pull request:
https://github.com/apache/incubator-predictionio/pull/425#discussion_r135429437
--- Diff:
core/src/main/scala/org/apache/predictionio/controller/PAlgorithm.scala ---
@@ -115,15 +115,12 @@ abstract class PAlgorithm[PD, M, Q, P]
algoParams: Params,
bm: Any): Any = {
val m = bm.asInstanceOf[M]
-if (m.isInstanceOf[PersistentModel[_]]) {
- if (m.asInstanceOf[PersistentModel[Params]].save(
-modelId, algoParams, sc)) {
-PersistentModelManifest(className = m.getClass.getName)
- } else {
-()
- }
-} else {
- ()
+m match {
+ case m: PersistentModel[Params] @unchecked =>
+if(m.save(modelId, algoParams, sc)){
+ PersistentModelManifest(className = m.getClass.getName)
+} else ()
+ case _ => ()
--- End diff --
I got it.
---
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] incubator-predictionio pull request #425: [PIO-110] Refactoring
Github user takezoe commented on a diff in the pull request:
https://github.com/apache/incubator-predictionio/pull/425#discussion_r135380673
--- Diff:
core/src/main/scala/org/apache/predictionio/controller/PAlgorithm.scala ---
@@ -115,15 +115,12 @@ abstract class PAlgorithm[PD, M, Q, P]
algoParams: Params,
bm: Any): Any = {
val m = bm.asInstanceOf[M]
-if (m.isInstanceOf[PersistentModel[_]]) {
- if (m.asInstanceOf[PersistentModel[Params]].save(
-modelId, algoParams, sc)) {
-PersistentModelManifest(className = m.getClass.getName)
- } else {
-()
- }
-} else {
- ()
+m match {
+ case m: PersistentModel[Params] @unchecked =>
+if(m.save(modelId, algoParams, sc)){
+ PersistentModelManifest(className = m.getClass.getName)
+} else ()
+ case _ => ()
--- End diff --
> This code will be simpler to read by joining the if guard with the case
statement.
Yes, but I want to keep the form of code as same as other algorithms.
> Which is correct?
I don't know. In any case, I think that I shouldn't modify current behavior
in this pull request because this pull request is for refactoring.
---
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] incubator-predictionio pull request #425: [PIO-110] Refactoring
Github user shimamoto commented on a diff in the pull request:
https://github.com/apache/incubator-predictionio/pull/425#discussion_r135180991
--- Diff:
data/src/main/scala/org/apache/predictionio/data/api/Webhooks.scala ---
@@ -62,22 +53,23 @@ private[predictionio] object Webhooks {
}
eventFuture.flatMap { eventOpt =>
- if (eventOpt.isEmpty) {
-Future successful {
- val message = s"webhooks connection for ${web} is not supported."
- (StatusCodes.NotFound, Map("message" -> message))
-}
- } else {
-val event = eventOpt.get
-val data = eventClient.futureInsert(event, appId, channelId).map {
id =>
- val result = (StatusCodes.Created, Map("eventId" -> s"${id}"))
-
- if (stats) {
-statsActorRef ! Bookkeeping(appId, result._1, event)
+ eventOpt match {
+case None =>
+ Future successful {
+val message = s"webhooks connection for ${web} is not
supported."
+(StatusCodes.NotFound, Map("message" -> message))
}
- result
-}
-data
+case Some(event) =>
+ val event = eventOpt.get
--- End diff --
It does not need it.
---
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] incubator-predictionio pull request #425: [PIO-110] Refactoring
Github user shimamoto commented on a diff in the pull request:
https://github.com/apache/incubator-predictionio/pull/425#discussion_r135187599
--- Diff:
storage/hbase/src/main/scala/org/apache/predictionio/data/storage/hbase/HBEventsUtil.scala
---
@@ -376,32 +375,30 @@ object HBEventsUtil {
}
targetEntityType.foreach { tetOpt =>
- if (tetOpt.isEmpty) {
-val filter = createSkipRowIfColumnExistFilter("targetEntityType")
-filters.addFilter(filter)
- } else {
-tetOpt.foreach { tet =>
+ tetOpt match {
+case None =>
+ val filter = createSkipRowIfColumnExistFilter("targetEntityType")
+ filters.addFilter(filter)
+case Some(tet) =>
--- End diff --
Same above.
---
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] incubator-predictionio pull request #425: [PIO-110] Refactoring
Github user shimamoto commented on a diff in the pull request:
https://github.com/apache/incubator-predictionio/pull/425#discussion_r135181421
--- Diff:
data/src/main/scala/org/apache/predictionio/data/api/Webhooks.scala ---
@@ -115,22 +107,22 @@ private[predictionio] object Webhooks {
}
eventFuture.flatMap { eventOpt =>
- if (eventOpt.isEmpty) {
-Future {
- val message = s"webhooks connection for ${web} is not supported."
- (StatusCodes.NotFound, Map("message" -> message))
-}
- } else {
-val event = eventOpt.get
-val data = eventClient.futureInsert(event, appId, channelId).map {
id =>
- val result = (StatusCodes.Created, Map("eventId" -> s"${id}"))
-
- if (stats) {
-statsActorRef ! Bookkeeping(appId, result._1, event)
+ eventOpt match {
+case None =>
+ Future {
+val message = s"webhooks connection for ${web} is not
supported."
+(StatusCodes.NotFound, Map("message" -> message))
+ }
+case Some(event) =>
+ val data = eventClient.futureInsert(event, appId, channelId).map
{ id =>
+val result = (StatusCodes.Created, Map("eventId" -> s"${id}"))
+
+if (stats) {
+ statsActorRef ! Bookkeeping(appId, result._1, event)
+}
+result
--- End diff --
Same above.
---
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] incubator-predictionio pull request #425: [PIO-110] Refactoring
Github user shimamoto commented on a diff in the pull request:
https://github.com/apache/incubator-predictionio/pull/425#discussion_r135187633
--- Diff:
storage/hbase/src/main/scala/org/apache/predictionio/data/storage/hbase/HBEventsUtil.scala
---
@@ -376,32 +375,30 @@ object HBEventsUtil {
}
targetEntityType.foreach { tetOpt =>
- if (tetOpt.isEmpty) {
-val filter = createSkipRowIfColumnExistFilter("targetEntityType")
-filters.addFilter(filter)
- } else {
-tetOpt.foreach { tet =>
+ tetOpt match {
+case None =>
+ val filter = createSkipRowIfColumnExistFilter("targetEntityType")
+ filters.addFilter(filter)
+case Some(tet) =>
val filter = createBinaryFilter(
"targetEntityType", Bytes.toBytes(tet))
// the entire row will be skipped if the column is not found.
filter.setFilterIfMissing(true)
filters.addFilter(filter)
-}
}
}
targetEntityId.foreach { teidOpt =>
- if (teidOpt.isEmpty) {
-val filter = createSkipRowIfColumnExistFilter("targetEntityId")
-filters.addFilter(filter)
- } else {
-teidOpt.foreach { teid =>
+ teidOpt match {
+case None =>
+ val filter = createSkipRowIfColumnExistFilter("targetEntityId")
+ filters.addFilter(filter)
+case Some(teid) =>
--- End diff --
Same above.
---
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] incubator-predictionio pull request #425: [PIO-110] Refactoring
Github user shimamoto commented on a diff in the pull request:
https://github.com/apache/incubator-predictionio/pull/425#discussion_r135175043
--- Diff:
core/src/main/scala/org/apache/predictionio/controller/PAlgorithm.scala ---
@@ -115,15 +115,12 @@ abstract class PAlgorithm[PD, M, Q, P]
algoParams: Params,
bm: Any): Any = {
val m = bm.asInstanceOf[M]
-if (m.isInstanceOf[PersistentModel[_]]) {
- if (m.asInstanceOf[PersistentModel[Params]].save(
-modelId, algoParams, sc)) {
-PersistentModelManifest(className = m.getClass.getName)
- } else {
-()
- }
-} else {
- ()
+m match {
+ case m: PersistentModel[Params] @unchecked =>
+if(m.save(modelId, algoParams, sc)){
+ PersistentModelManifest(className = m.getClass.getName)
+} else ()
+ case _ => ()
--- End diff --
This code will be simpler to read by joining the if guard with the case
statement.
```scala
case m: PersistentModel[Params] @unchecked
if m.save(modelId, algoParams, sc) => ...
case _ => ...
```
But it looks this behavior differs from other algorithms (LAlgorithm.scala
and P2LAlgorithm.scala). Which is correct?
---
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] incubator-predictionio pull request #425: [PIO-110] Refactoring
Github user shimamoto commented on a diff in the pull request:
https://github.com/apache/incubator-predictionio/pull/425#discussion_r135180795
--- Diff:
data/src/main/scala/org/apache/predictionio/data/api/Webhooks.scala ---
@@ -62,22 +53,23 @@ private[predictionio] object Webhooks {
}
eventFuture.flatMap { eventOpt =>
- if (eventOpt.isEmpty) {
-Future successful {
- val message = s"webhooks connection for ${web} is not supported."
- (StatusCodes.NotFound, Map("message" -> message))
-}
- } else {
-val event = eventOpt.get
-val data = eventClient.futureInsert(event, appId, channelId).map {
id =>
- val result = (StatusCodes.Created, Map("eventId" -> s"${id}"))
-
- if (stats) {
-statsActorRef ! Bookkeeping(appId, result._1, event)
+ eventOpt match {
+case None =>
+ Future successful {
+val message = s"webhooks connection for ${web} is not
supported."
+(StatusCodes.NotFound, Map("message" -> message))
--- End diff --
It's better to use in function args for pattern matching.
```scala
eventFuture.flatMap {
case None => ...
case Some(event) => ...
}
```
---
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] incubator-predictionio pull request #425: [PIO-110] Refactoring
Github user shimamoto commented on a diff in the pull request:
https://github.com/apache/incubator-predictionio/pull/425#discussion_r135191103
--- Diff:
data/src/main/scala/org/apache/predictionio/data/api/Webhooks.scala ---
@@ -115,22 +107,22 @@ private[predictionio] object Webhooks {
}
eventFuture.flatMap { eventOpt =>
- if (eventOpt.isEmpty) {
-Future {
- val message = s"webhooks connection for ${web} is not supported."
- (StatusCodes.NotFound, Map("message" -> message))
-}
- } else {
-val event = eventOpt.get
-val data = eventClient.futureInsert(event, appId, channelId).map {
id =>
- val result = (StatusCodes.Created, Map("eventId" -> s"${id}"))
-
- if (stats) {
-statsActorRef ! Bookkeeping(appId, result._1, event)
+ eventOpt match {
+case None =>
+ Future {
+val message = s"webhooks connection for ${web} is not
supported."
+(StatusCodes.NotFound, Map("message" -> message))
+ }
--- End diff --
Originally `Future {...}`, but here it looks good to use `Future
successful`.
---
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] incubator-predictionio pull request #425: [PIO-110] Refactoring
GitHub user takezoe reopened a pull request: https://github.com/apache/incubator-predictionio/pull/425 [PIO-110] Refactoring You can merge this pull request into a Git repository by running: $ git pull https://github.com/takezoe/incubator-predictionio refactor-common-code Alternatively you can review and apply these changes as the patch at: https://github.com/apache/incubator-predictionio/pull/425.patch To close this pull request, make a commit to your master/trunk branch with (at least) the following in the commit message: This closes #425 commit 24e1ec7a626857d320d69d8d09b68daead818831 Author: Naoki Takezoe Date: 2017-08-23T05:42:52Z [PIO-110] Refactoring --- 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] incubator-predictionio pull request #425: [PIO-110] Refactoring
Github user takezoe closed the pull request at: https://github.com/apache/incubator-predictionio/pull/425 --- 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] incubator-predictionio pull request #425: [PIO-110]Refactoring
GitHub user takezoe opened a pull request: https://github.com/apache/incubator-predictionio/pull/425 [PIO-110]Refactoring You can merge this pull request into a Git repository by running: $ git pull https://github.com/takezoe/incubator-predictionio refactor-common-code Alternatively you can review and apply these changes as the patch at: https://github.com/apache/incubator-predictionio/pull/425.patch To close this pull request, make a commit to your master/trunk branch with (at least) the following in the commit message: This closes #425 commit 8a2aa88148b96625764b5c619614064136344e66 Author: Naoki Takezoe Date: 2017-08-23T05:42:52Z [PIO-110]Refactoring --- 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. ---
