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

Changes for reading ADL AOM JSON Files #569

Merged
merged 4 commits into from
Apr 1, 2024
Merged

Conversation

David-N-Perkins
Copy link

@David-N-Perkins David-N-Perkins commented Feb 13, 2024

These changes were made to enable parsing of AOM JSON files created by the ADL Workbench.
They include:

  • Changing the standard type name EXPR_LITERAL to EXPR_LEAF
  • Adding C_STRING to the Expression Type enum
  • Enabling case insensitive enumeration mapping in Jackson
  • Allowing single numbers for parsing Multiplicity Intervals
  • A unit test with an example AOM JSON file

wolandscat and others added 3 commits October 12, 2023 11:25
rename standard type name EXPR_LITERAL to EXPR_LEAF;
add C_STRING as valid Expression Type;
enable Enum case insensitive mapping;
allow single number Multiplicity Intervals;
add test cases for S2 AOM JSON
@David-N-Perkins David-N-Perkins changed the title Changes for reading AWB AOM JSON Files Changes for reading ADL AOM JSON Files Feb 13, 2024
rename Expression Type  C_STRING string representation to CString
@@ -5,7 +5,7 @@
* Created by pieter.bos on 27/10/15.
*/
public enum ExpressionType {
BOOLEAN, STRING, INTEGER, REAL, DATE, TIME, DATETIME, DURATION;
BOOLEAN, STRING, INTEGER, REAL, DATE, TIME, DATETIME, DURATION, C_STRING;
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I am not sure if C_STRING should be added here. Expression type is used as the result of an expression when evaluating.
I cannot think of an expression with result type C_STRING. Would that not be a boolean, whether it matches or not?

is there a reason in particular why this type would be needed here?

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This is the type of the ARCHETYPE_ID_CONSTRAINT atom, used with the matches operator. In general there can be a subtree whose construction is:

op = matches; type = Boolean
    opd_left = /path/to/some/value; type = <some primitive type>
    opd_right = C_XXX constraint object; type = <constraint for primitive type>

So for the specific case of matching archetype id, it is

op = matches; type = Boolean
    opd_left = /archetype_id/value ; type = String
    opd_right = C_STRING regex to match archetype id; constraint object; type = C_STRING

As far as we can tell, the enum needs to have C_STRING in it, because without it, it doesn't allow any operand (or other node) to be a C_STRING. Most likely, all the C_XXX primitive types should be there, but we don't use them, so for now at least it won't matter. But the matches operator always has a sub-tree of the first form above, with a constraint matcher node on the right hand side.

Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I guess that's correct - I guess we just don't always fill this. But then you also need all the other C_-types that can be expressed in the rules, at least C_TERMINOLOGY_CODE and C_BOOLEAN. Should not block these changes of course.

@wolandscat wolandscat merged commit 1e081b9 into openEHR:master Apr 1, 2024
1 check passed
Copy link
Collaborator

@pieterbos pieterbos left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Good changes, 👍.

@@ -5,7 +5,7 @@
* Created by pieter.bos on 27/10/15.
*/
public enum ExpressionType {
BOOLEAN, STRING, INTEGER, REAL, DATE, TIME, DATETIME, DURATION;
BOOLEAN, STRING, INTEGER, REAL, DATE, TIME, DATETIME, DURATION, C_STRING;
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I guess that's correct - I guess we just don't always fill this. But then you also need all the other C_-types that can be expressed in the rules, at least C_TERMINOLOGY_CODE and C_BOOLEAN. Should not block these changes of course.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants