Conversation
UmlToCifTranslator: translate activity requirements
|
Documentation to be updated as well. |
There was a problem hiding this comment.
Need to update the method JavaDoc.
| } | ||
|
|
||
| // Translate activity requirements. | ||
| for (Constraint umlConstraint: activity.getOwnedRules()) { |
There was a problem hiding this comment.
Is it sufficient to translate the requirements like this? Should we also include all requirements of other activities that we call, etc? (all other 'relevant' activities)
There was a problem hiding this comment.
I did this to be consistent with the occurrence constraints: these are considered only during the activity synthesis, not when an activity is called. Better to have a conversation offline about this.
| return constraint.getContext() instanceof PrimitiveType; | ||
| } | ||
|
|
||
| public static boolean isActivityRequirement(Constraint constraint) { |
There was a problem hiding this comment.
Move isActivityRequirement to just after isActivityPostconditionConstraint.
| return constraint.getContext() instanceof PrimitiveType; | ||
| } | ||
|
|
||
| public static boolean isActivityRequirement(Constraint constraint) { |
There was a problem hiding this comment.
Rename isActivityRequirement to isActivityRequirementConstraint, to match other methods?
|
|
||
| public static boolean isActivityRequirement(Constraint constraint) { | ||
| return constraint.getContext() instanceof Activity activity | ||
| // It is the correct type of constraint. |
There was a problem hiding this comment.
Can't we check that it is a FormalConstraint?
There was a problem hiding this comment.
It was my first choice, but I then wrote it this way to be consistent with the rest of the class and structure that we have e.g., in the validator. To be honest, if I write a isRequirementConstraint method in PokaYokeUmlProfileUtil, this method in CifContext is not needed at all. But it will not be consistent with the rest of validation, for instance.
I will remove this method and add a new one in PokaYokeUmlProfileUtil, and let's see how it looks. I think it looks better; but if consistency with the rest is more important we can revert back to this.
There was a problem hiding this comment.
Rename to isClassRequirementConstraint?
| .filter(r -> CifContext.isActivityRequirement(r)).map(Constraint.class::cast) | ||
| .collect(Collectors.toCollection(LinkedHashSet::new)); | ||
|
|
||
| if (!members.equals(Sets.union(Sets.union(preAndPostconditions, intervalConstraints), activityRequirements))) { |
There was a problem hiding this comment.
I think Sets.union supports also giving 3 sets instead of 2, right?
There was a problem hiding this comment.
Not sure how to make it work, so I created a set union with stream.
| checkValidActivityPrePostconditionConstraint(constraint); | ||
| } else if (CifContext.isClassConstraint(constraint)) { | ||
| checkValidClassConstraint(constraint); | ||
| } else if (CifContext.isClassConstraint(constraint) || (CifContext.isActivityRequirement(constraint))) { |
There was a problem hiding this comment.
Does it still make sense to have CifContext.isClassConstraint? Should we rename it to CifContext.isClassRequirementConstraint?
There was a problem hiding this comment.
See previous comments.
| } | ||
|
|
||
| private void checkValidClassConstraint(Constraint constraint) { | ||
| private void checkValidClassOrActivityConstraint(Constraint constraint) { |
There was a problem hiding this comment.
This is only requirements, from classes and activities, right? So, maybe rename to checkValidRequirementConstraint?
There was a problem hiding this comment.
Why check this with a global scope? Why not in the scope of the activity, if it is in an activity?
|
Does editing these new constraints with the SynthML tab work? |
...yoke.transform.uml2cif/src/com/github/tno/pokayoke/transform/uml2cif/UmlToCifTranslator.java
Outdated
Show resolved
Hide resolved
| @@ -529,7 +528,7 @@ public static List<Stereotype> getSupportedConstraintStereotypes(Constraint cons | |||
| } else if (isPostconditionConstraint(constraint)) { | |||
| return List.of(getStereotype(constraint, ST_POSTCONDITION)); | |||
| } else if (isClassRequirement(constraint)) { | |||
There was a problem hiding this comment.
I always forget that Activity inherits from Class. Super non-intuitive. I'd maybe rename the method to isClassOrActivityRequirementConstraint, and there add a comment in the body like // Note that 'Activity' inherits from 'Class'. or so?
| return appliedStereotypes.get(0).getName().equals(ST_USAGE_PRECONDITION); | ||
| } | ||
|
|
||
| public static boolean isRequirementConstraint(Constraint constraint) { |
There was a problem hiding this comment.
Maybe move isPostconditionConstraint and isClassRequirement to just above isSynthesisPrecondition, to have methods with similar implementation close together?
| return appliedStereotypes.get(0).getName().equals(ST_USAGE_PRECONDITION); | ||
| } | ||
|
|
||
| public static boolean isRequirementConstraint(Constraint constraint) { |
There was a problem hiding this comment.
There are three methods for 'where' the constraint is (precondition/postcondition of activity, rule of a class/activity), and three methods about what kind (synthesis pre, usage pre, requirement).
I'm thinking the names could be clearer. Maybe:
- isSynthesisPreconditionConstraint/isUsagePreconditionConstraint/isRequirementConstraint to just check the stereotype that is applied, more or less as we have now (but all end their name with
Constraint). - isPreconditionConstraint/isPostconditionConstraint/isClassRequirementConstraint -> isContainedAsActivityPrecondition/isContainedAsActivityPostcondition/isContainedAsClassOrActivityOwnedRule to see where they are contained in the model. That distinguishes their names better.
...profile.util/src/com/github/tno/synthml/uml/profile/validation/PokaYokeProfileValidator.java
Outdated
Show resolved
Hide resolved
| } else if (CifContext.isClassConstraint(constraint)) { | ||
| checkValidClassConstraint(constraint); | ||
| } else if (CifContext.isClassConstraint(constraint) | ||
| || (PokaYokeUmlProfileUtil.isRequirementConstraint(constraint))) |
There was a problem hiding this comment.
Can it ever be a requirement constraint and not a class constraint? What constraints that we support are allowed in classes that are not requirements?
In other words, can't we only check for being a requirement constraint, as that also matches checkValidRequirementConstraint?
There was a problem hiding this comment.
This should be fixed with the new methods.
...profile.util/src/com/github/tno/synthml/uml/profile/validation/PokaYokeProfileValidator.java
Outdated
Show resolved
Hide resolved
| } | ||
|
|
||
| @Check | ||
| private void checkValidConstraint(Constraint constraint) { |
There was a problem hiding this comment.
I wonder if we should check here for what kind of constraint it is, and then in the methods we invoke, that they are in the right place (a supported place), maybe? Now it seems mixed.
Together with the renaming of the 6 methods in PokaYokeUmlProfileUtil (see other comment), I think we could probably remove the CifContext methods, and make it a lot cleaner. I don't think it would give a lot of changes in many places. Maybe a few of the context methods need an extra method in the profile util then, though.
There was a problem hiding this comment.
Added new methods in PokaYokeUmlProfileUtil and removed a few from CifContext.
Co-authored-by: Dennis Hendriks <dh_tue@hotmail.com>
UmlToCifTranslator, FlattenUmlActivity: rename methods
- make methods public - add occurrence constraint and primitive type constraint methods PokaYokeProfileValidator: use the new methods
UmlToCameoTransformer, Uml2GalTranslator, PokaYokeProfileValidator: use the PokaYokeUmlProfileUtil method
AbstractActivityDependencyOrderer: use the PokaYokeUmlProfileUtil method
constraint PokaYokeProfileValidator: use the PokaYokeUmlProfileUtil methods
|
Ready for review, best commit-by-commit. |




Closes #683.
I will add a regression test once the changes are approved.