Skip to content
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

Merged
merged 3 commits into from
May 16, 2024
Merged

Conversation

enricocolasante
Copy link
Collaborator

@enricocolasante enricocolasante commented May 14, 2024

Evaluate methods now have a triggerEnvironment parameter with a default value of getEnvironment().

Copy link
Member

@vgarciabnz vgarciabnz left a 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
Copy link
Member

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
Copy link
Member

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

Copy link
Collaborator Author

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

@enricocolasante enricocolasante merged commit cb47f8c into beta May 16, 2024
1 check passed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Development

Successfully merging this pull request may close these issues.

None yet

3 participants