-
Notifications
You must be signed in to change notification settings - Fork 10
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
Feature/10 gateway extension test #12
Conversation
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Massive bunch of documentation arguments. Think those should be resolved prior to my approval. Conceptually, I do not see any issues with the provided code, hence I am not being harsh we a "request changes".
* #%L | ||
* Axon Framework - Kotlin Extension | ||
* %% | ||
* Copyright (C) 2019 AxonIQ | ||
* %% | ||
* Licensed under the Apache License, Version 2.0 (the "License"); | ||
* you may not use this file except in compliance with the License. | ||
* You may obtain a copy of the License at | ||
* | ||
* http://www.apache.org/licenses/LICENSE-2.0 | ||
* | ||
* Unless required by applicable law or agreed to in writing, software | ||
* distributed under the License is distributed on an "AS IS" BASIS, | ||
* WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied. | ||
* See the License for the specific language governing permissions and | ||
* limitations under the License. | ||
* #L% |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Are the #, % and L characters intended in the copyright notice?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
import org.axonframework.messaging.MetaData | ||
|
||
/** | ||
* Implementation of the [CommandCallback] that is appropriate for dedicated [onError] and [onSuccess] callbacks |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Do we want to point out the authors of a class for this extension?
import org.axonframework.messaging.MetaData | ||
|
||
/** | ||
* Implementation of the [CommandCallback] that is appropriate for dedicated [onError] and [onSuccess] callbacks |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Do we have a milestone/version plan for this extension? Or, differently put, wouldn't a @since
tag (or KotlinDocs equivalent) be in place?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Created #18 to handle this topic
kotlin/src/main/kotlin/org/axonframework/extensions/kotlin/CombiningCommandCallback.kt
Outdated
Show resolved
Hide resolved
import org.axonframework.commandhandling.CommandMessage | ||
import org.axonframework.commandhandling.gateway.CommandGateway | ||
import org.axonframework.messaging.MetaData | ||
import java.util.concurrent.TimeUnit | ||
|
||
/** | ||
* Extension for command gateway. | ||
* Callback-style send with dedicated on-success and on-error functions (defaults do nothing) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This is still an extension function of the CommandGateway
it's send(C, CommandCallback<? super C, ? super R>)
, right? I think the documentation should specify that, as well as linking to the original method's documentation with the bracket ([CommandGateway.send]
notation.
kotlin/src/test/kotlin/org/axonframework/extensions/kotlin/QueryGatewayExtensionsTest.kt
Show resolved
Hide resolved
private val exceptionalCommandResultMessage = GenericCommandResultMessage.asCommandResultMessage<String>(CommandDispatchException("Exception message")) | ||
private val metaData = MetaData.with("key", "value") | ||
|
||
@Test |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Should've told you earlier (so sorry for this), but the framework has moved to JUnit 5 entirely at the moment and the majority of the Kafka Extension has been adjusted. Might be beneficial take this stance for the Kotlin extension too.
|
||
} | ||
|
||
private class ExampleQuery(val value: Number) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Super nit: shouldn't this be a data class
too?
/*- | ||
* #%L | ||
* Axon Framework - Kotlin Extension | ||
* %% | ||
* Copyright (C) 2019 AxonIQ | ||
* %% | ||
* Licensed under the Apache License, Version 2.0 (the "License"); | ||
* you may not use this file except in compliance with the License. | ||
* You may obtain a copy of the License at | ||
* | ||
* http://www.apache.org/licenses/LICENSE-2.0 | ||
* | ||
* Unless required by applicable law or agreed to in writing, software | ||
* distributed under the License is distributed on an "AS IS" BASIS, | ||
* WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied. | ||
* See the License for the specific language governing permissions and | ||
* limitations under the License. | ||
* #L% | ||
*/ |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Are the #, % and L characters intended in the copyright notice?
import org.axonframework.messaging.responsetypes.ResponseType | ||
import java.util.* | ||
|
||
internal fun <T> MockKVerificationScope.responseTypeOfMatcher(clazz: Class<T>) = match { type: ResponseType<T> -> type.expectedResponseType == clazz } |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I think these methods deserve some documentation.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Looks good to me 👍
@@ -19,43 +19,28 @@ | |||
*/ | |||
package org.axonframework.extensions.kotlin | |||
|
|||
import org.axonframework.commandhandling.CommandCallback | |||
import org.axonframework.commandhandling.CommandMessage | |||
import org.axonframework.commandhandling.gateway.CommandGateway | |||
import org.axonframework.messaging.MetaData | |||
import java.util.concurrent.TimeUnit | |||
|
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I am trusting in your expertise here @sandjelkovic. Thus, removing it should be fine then.
This PR will resolve #10
CombiningCommandCallback
for easier testability