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

Split flattener into separate parts #7

Merged
merged 4 commits into from
Dec 3, 2017
Merged

Split flattener into separate parts #7

merged 4 commits into from
Dec 3, 2017

Conversation

pieterbos
Copy link
Collaborator

@pieterbos pieterbos commented Nov 28, 2017

Splits flattener in separate parts for better readability, and a few minor renames for easier to read code.

In addition, a few minor bug fixes:

  • referenceModelpropertyMultiplicity did not work on paths, only property names. That's been fixed.
  • serialization of differential paths was broken, only the last attributename was serialized. That's been fixed.

And new tests:

  • new tests for effectiveOccurrences

@pieterbos pieterbos mentioned this pull request Nov 29, 2017
@pieterbos pieterbos changed the base branch from correct_sibling_order to master November 29, 2017 14:29
@pieterbos pieterbos changed the title Split flattener in separate parts Split flattener into separate parts Nov 30, 2017
Copy link

@cnanjo cnanjo left a comment

Choose a reason for hiding this comment

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

Hello Pieter,
I am not an expert on the archetype part of the library so my comments will be limited to more general things.

The proposed refactoring makes sense. It leads to a better separation of concerns.

In the following code snippet, we have an unknown state. Should we somehow log this case? Right now, it is silently ignored so we don't know how often we might hit this case.

} else if (object instanceof CObject) {
//TODO: what does this mean?
}

The package still refers to nedap. Should it be changed to openehr?

com.nedap.archie
org.openehr.archie

I suppose you are waiting to see what the new library name will be. If so, please ignore.

In the following snippet:

attributeInParent.setExistence(FlattenerUtil.getPossiblyOverridenValue(attributeInParent.getExistence(), attributeInSpecialization.getExistence())); attributeInParent.setCardinality(FlattenerUtil.getPossiblyOverridenValue(attributeInParent.getCardinality(), attributeInSpecialization.getCardinality()));

Where do you check that the cardinality redefinition in the child is valid? That is, so it does not loosen the cardinality of the parent? Same question for existence. If you do it elsewhere, please ignore comment.

Thank you,

Claude.

@pieterbos
Copy link
Collaborator Author

Thanks for the review!

Unknown case should not occur - logged or exception

Package will be changed later in one big pr

It’s assumed you have run archetypes through the validator. That one checks the cardinality.

@pieterbos
Copy link
Collaborator Author

pieterbos commented Dec 3, 2017

About archetype validator and flattener: we made the flattener work on several kinds of invalid input. That could have its uses in modeling tools (‘altough you have not yet made this archetype valid, this gives an idea of what the flat form will look like. Now go fix those errors ’). I’m not sure whether it’s a good idea or not.

The archetype validator of course is strict, as it should be. So one should usually first run the validator, then the flattener.

An easy API to do that is InMemoryFullArchetypeRepository.compile(). it validates all archetypes in the repository and flattens only the archetypes that are valid enough to be flattened (passed phase 1 and phase 2 validation, as listed in the spec). For the others it will list error messages.

@pieterbos pieterbos merged commit eadc8f5 into master Dec 3, 2017
@pieterbos pieterbos deleted the split_flattener branch January 14, 2018 20:07
wolandscat added a commit that referenced this pull request Oct 14, 2024
Local build-related changes.
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.

2 participants