feat: Add AndroidCustomIntent evaluation mode#86
Conversation
| } | ||
|
|
||
| public final class org/hisp/dhis/lib/expression/spi/DataItemType : java/lang/Enum { | ||
| public static final field ANDROID_CUSTOM_INTENT Lorg/hisp/dhis/lib/expression/spi/DataItemType; |
There was a problem hiding this comment.
There are many things in the public API that must be internal, such as this enum. I didn't want to make it internal just to be consistent with the rest of the modes. @jbee should we create a task to clean this up? I can push it.
There was a problem hiding this comment.
Yes, new task. I think I first should also do the PR with the unified context map. Then we can do the cleanup.
| import kotlin.js.JsExport | ||
|
|
||
| @JsExport | ||
| enum class AndroidCustomIntentVariable { |
There was a problem hiding this comment.
I decided to create an enum just to be able to validate the expression.
There was a problem hiding this comment.
Using an enum sure makes VAR{...} just usable by android (unless someone has the exact use case of one or more of these variables). It is still fine but it is somewhat of an odd mix to pair a generic name with a very specific usability. Anyhow, the expression syntax is already odd so I think we just go with what you like better. If we ever want to make something that composes better we would have to invent a 2.0 version anyhow.
FYI: When I make this into the context key-value the key would be a wrapper on this enum or the enum itself if it can implement the interface. For the value we should talk about if you rather want a wrapper on Any or a wrapper having 1 field per possible return type.
There was a problem hiding this comment.
Makes sense. This variable name can be changed later if we find that is messing things up. The only client will probably be the Android app, unless for a while.
Anyway, wouldn't be possible to create another node that uses VAR{} and links to another enum of variables? I got the feeling the other day after our conversation that it was possible to override the those symbols and link them to a different node. Just for curiosity.
jbee
left a comment
jbee
left a comment
There was a problem hiding this comment.
I think that should work :)
| ValueType.DATE), | ||
|
|
||
| // android custom intent request parameters | ||
| ANDROID_CUSTOM_INTENT_EXPRESSION(ExpressionGrammar.AndroidCustomIntentMode, ValueType.NUMBER); |
There was a problem hiding this comment.
When we discussed this you said you pass along the value typed using different types. In that case all the types you use should be listed here. At least STRING, NUMBER and BOOLEAN seem likely.
If I recall correctly this list is for validation so we can check that the result we got matches the expectation. If you don't list a type here but the expression does compute to a value of that type and you validate you will get an error.
There was a problem hiding this comment.
You are right, I forgot to add them here, I will do it.
There are tests already in place that verify the return value type. Shouldn't they fail?
| } | ||
|
|
||
| public final class org/hisp/dhis/lib/expression/spi/DataItemType : java/lang/Enum { | ||
| public static final field ANDROID_CUSTOM_INTENT Lorg/hisp/dhis/lib/expression/spi/DataItemType; |
There was a problem hiding this comment.
Yes, new task. I think I first should also do the PR with the unified context map. Then we can do the cleanup.
| import kotlin.js.JsExport | ||
|
|
||
| @JsExport | ||
| enum class AndroidCustomIntentVariable { |
There was a problem hiding this comment.
Using an enum sure makes VAR{...} just usable by android (unless someone has the exact use case of one or more of these variables). It is still fine but it is somewhat of an odd mix to pair a generic name with a very specific usability. Anyhow, the expression syntax is already odd so I think we just go with what you like better. If we ever want to make something that composes better we would have to invent a 2.0 version anyhow.
FYI: When I make this into the context key-value the key would be a wrapper on this enum or the enum itself if it can implement the interface. For the value we should talk about if you rather want a wrapper on Any or a wrapper having 1 field per possible return type.
|
2 participants
This PR adds a new evaluation mode for Android Custom Intents.
There is a predefined list of supported variables (
AndroidCustomIntentVariable). Their value must be provided by the client using theExpressionData.programVariableValuesproperty. This is an abuse of the model, but it was agreed to use this property to avoid breaking changes in the API.The mode includes some D2 functions, which are needed in the known use cases of custom intents.