-
Notifications
You must be signed in to change notification settings - Fork 16
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
fix: Fix native trigger environment #135
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.
Looks good to me. I have added a small comment but I leave it approved anyway (I am going to be out the rest of the week)
import org.hisp.dhis.rules.models.* | ||
import org.hisp.dhis.rules.models.TriggerEnvironment.SERVER |
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.
These two imports are not used and could be removed
import org.hisp.dhis.rules.models.* | ||
import org.hisp.dhis.rules.models.TriggerEnvironment.SERVER | ||
import kotlin.jvm.JvmOverloads |
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.
Well, after a second thought, I do think we need to annotate the methods as overloaded if we want to make it optional in Java. In Kotlin, the last parameter would be optional, but in Java it will be mandatory unless the methods are mark as overloaded
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.
Unfortunately, @jvmoverloads is not allowed on interfaces and default parameters are not allowed on overriding methods.
So I am manually creating the overload methods.
Another option would be not to use an interface.
I am merging this so I can test the artifact but then we can discuss the best solution
Evaluate methods now have a
triggerEnvironment
parameter with a default value ofgetEnvironment()
.