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

implemented a parent-child relation between spec and impl #20

Open
wants to merge 1 commit into
base: master
Choose a base branch
from

Conversation

FedorSmirnov89
Copy link
Member

Made the specification class extend 'Element' instead of only implementing 'IAttributes'. Each implementation now has a reference to the specification as its parent. This allows to (1) conveniently access the spec from the implementation (e.g. during the evaluation) and (2) to explore parameters per implementation by just setting a parameter as an attribute of the specification.

Copy link
Member

@michaelhglass michaelhglass left a comment

Choose a reason for hiding this comment

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

Please elaborate on the fixed ID of the specification that currently cannot be changed by the user.

@@ -41,8 +41,8 @@
public class Models {
Copy link
Member

Choose a reason for hiding this comment

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

Please check for formatting style. It seems all the comments are touched by your style.

@@ -348,6 +355,11 @@ public Resource getDest() {
for (Mapping<Task, Resource> mapping : mappings) {
elements.put(mapping.getId(), mapping);
}
if (elements.containsKey(Specification.SPECIFICATION_ELEMENT_ID)) {
Copy link
Member

Choose a reason for hiding this comment

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

See comment in class Specification.


// the unique id of the specification element; no other element must have this id
public static final String SPECIFICATION_ELEMENT_ID = "specificationElementId";
Copy link
Member

Choose a reason for hiding this comment

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

Why do you opt for a static ID of the specification that is fixed in the code without also exposing a constructor where the ID could be changed? Is the plan to hide the ID of a specification from the user? If so, why?

Copy link
Member Author

@FedorSmirnov89 FedorSmirnov89 Mar 26, 2019

Choose a reason for hiding this comment

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

Sorry about the thing with the comments. I will fix it.

About the specification ID:
In the current PR, the ID is a public static variable, so it is not about hiding it. Now that I think of it, I don't see any real reason to have a fixed spec ID. On the other hand, I also cannot think of a situation where giving the specification a certain ID would be useful in any way (with it being unique for each exploration anyway).

The only situation where the ID may matter are potential name collisions where you could end up overwriting the spec in the Elements map. Having it as a constant, publically visible variable may reduce the risk of ID collisions, since there would be only one constant name to avoid when naming Elements.

All in all, I don't have a strong opinion into either direction. What do you think?

Copy link
Member

Choose a reason for hiding this comment

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

Currently, Element comes with a constructor Element(String id) such that it is in the hand of the user to possibly handle (or be aware of) id conflicts. On the other hand, this also enables to use IDs for the sake of input/output handling, GUIs etc etc. I think a reasonable solution would be to use a generic "specification" as ID if none is given or also expose the constructor with String id in case the user wants to set a specification name/id as can be done for all Elements.

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