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

Add feature model metamodel extension #132

Open
wants to merge 2 commits into
base: development
Choose a base branch
from

Conversation

mgv99
Copy link
Collaborator

@mgv99 mgv99 commented Dec 10, 2024

No description provided.

@mgv99 mgv99 requested a review from ivan-alfonso December 10, 2024 10:16
Copy link
Collaborator

Choose a reason for hiding this comment

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

Please add this line

from .feature_model import *

Copy link
Collaborator

Choose a reason for hiding this comment

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

Some comments:

1. Default Mutable Arguments in Constructors

There are empty lists assigned by default in some variables in the constructor methods. This can cause issues because lists are mutable objects shared between instances.

For example, in line 113:

features: list[Feature] = []

If you define the following:

f1 = FeatureGroup(kind="MANDATORY")
f1.features.append(1)
f2 = FeatureGroup(kind="MANDATORY")

Then f1.features will be the same as f2.features, which creates unintended shared states.

A simple fix for this issue is to use None as the default value and initialize the list inside the constructor:

def __init__(self, kind: str, features: List[Feature] = None):
    if features is None:
        features = []
    self.features: List[Feature] = features

2. Type Annotation

I think the standard way to define the code in line 139 is:

from typing import Union
self.value: Union[int, float, str] = None

3 value Attribute in FeatureConfiguration Constructor

The value attribute of the FeatureConfiguration class is not included in the __init__ method parameters. This means the value cannot be specified during the creation of an object. Is this omission intentional?

4 Missing relationship

It seems that the has_children relationship between FeatureGroup and Feature is either not directly implemented or not clearly defined.

Copy link
Collaborator

Choose a reason for hiding this comment

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

To maintain consistency with the rest of the BESSER metamodels, here are some suggestions for the metamodel figure:

  • The names of the associations or end names should not contain spaces. For example, update the names to use underscores (_) instead: Replace has features with has_features or simply features

  • The small arrow in the association names is unnecessary. Instead, position the label closer to the appropriate end of the association to indicate the direction. For example, move the has_features label closer to the Feature end to indicate that the list of Features can be accessed from FeatureGroup using this label.

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