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

[RFC] for single-attribute value types #409

Merged
merged 8 commits into from
Jan 22, 2024
Merged

Conversation

dirkriehle
Copy link
Member

Adds alternative syntax to current single-attribute value types and prepares the way for multi-attribute value types.

@github-actions
Copy link

github-actions bot commented Jul 29, 2023

CLA Assistant Lite bot All contributors have signed the CLA ✍️ ✅

@dirkriehle
Copy link
Member Author

I have read the CLA Document and I hereby sign the CLA

@rhazn
Copy link
Contributor

rhazn commented Jul 31, 2023

recheck (just for the bot to understand signing the CLA).

Copy link
Member

@georg-schwarz georg-schwarz left a comment

Choose a reason for hiding this comment

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

Love it!

I think we should consider adapting the constraints syntax to be better suited for the extension to multi-attribute valuetypes (see the comments).

}
```

This syntax is smart in that you don't have to list and name an attribute but rather rely on an implicit 'value' attribute. Still, I propose to make that attribute explicit. New syntax would be:
Copy link
Member

Choose a reason for hiding this comment

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

Does this implicitly mean that single-attribute valuetypes have to define the attribute value?

Copy link
Contributor

Choose a reason for hiding this comment

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

I don't think we should enforce this, the less naming magic the better. We can assume for a transition period that if a value type has only one attribute, that attribute is mapped to the value keyword in expressions for constraints.

Copy link
Member Author

Choose a reason for hiding this comment

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

In the current syntax, I think there is no name, because it is implicit? In my proposed syntax, you can give it any name you want. It does not have to be value.

Comment on lines +62 to +65
attributes: [
amount oftype decimal;
currency oftype Currency;
];
Copy link
Member

Choose a reason for hiding this comment

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

Seems very intuitive!

I'd like to show an alternative syntax for this that I personally like more:

valuetype Money {
  attribute amount oftype decimal;
  attribute currency oftype Currency;
}

This is probably sth very general to discuss on language design: keep the syntax flat or nested? I think here, the flat approach is more natural and similar to other languages.

Copy link
Contributor

Choose a reason for hiding this comment

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

We've used property instead of attributes in built-in blocks (https://github.com/jvalue/jayvee/tree/main/rfc/0008-builtin-blocktypes). I'd use the same naming here. For blocks, we decided properties are values an object owns intrinsically that you use in modeling, attributes is something you attribute to objects from the outside. The distinction is very faint but I would argue for consistency and reuse property here.

Otherwise I think the syntax is nice and would go for:

valuetype Money {
  property amount oftype decimal;
  property currency oftype Currency;
}

Copy link
Member Author

Choose a reason for hiding this comment

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

At first glance, I'm indifferent/don't see a difference between property and attribute. Is there a semantic difference?

Which ever keyword we use, I observe a pattern of singular plural i.e. property/properties and constraint/constraints.

My take is that users may want both (property for one new property, and properties for a list of properties, constraint for one constraint, and constraints for a set of constraints). If only one is possible, I'd go for singular i.e. Philip's suggestion

## Drawbacks

1. Introduces a redundant syntax,
2. Creates extra work/may be too difficult, and
Copy link
Member

Choose a reason for hiding this comment

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

I don't think it would be too difficult ;-)

One issue ahead is the attribute name value as we use value as keyword in expression-based constraints. These constraints have to be redesigned then to rather define parameters in a lambda-like way, which is a good idea anyway.


1. Introduces a redundant syntax,
2. Creates extra work/may be too difficult, and
3. May be disruptive to how the language currently works.
Copy link
Member

Choose a reason for hiding this comment

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

IMO it blends in well ;-)


## Drawbacks

1. Introduces a redundant syntax,
Copy link
Member

Choose a reason for hiding this comment

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

Alternatively, we could replace the current syntax with it?

Copy link
Contributor

Choose a reason for hiding this comment

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

I'd also replace the syntax.

Copy link
Member Author

Choose a reason for hiding this comment

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

The beauty of not much legacy yet ;-) Agreed.


## Possible Future Changes/Enhancements

This proposal is to prepare the way for multi-attribute value types.
Copy link
Member

Choose a reason for hiding this comment

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

I'd like to point out the interplay with generics that we currently need to implement (without behavior yet) to represent collections in the blocktype definition.

Here a generic example since I don't have a good usecase for it in mind yet.

valuetype MyGenericValuetype<T> {
  parameters: [
     x oftype text,
     y oftype T
  ]
}

Copy link
Member Author

Choose a reason for hiding this comment

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

We really should put this to the test in AMSE/SAKI/MADE. I have always suspected that beyond very simple uses of generics, folks don't get it/don't use it because it makes their brain jump out of its casing.

Copy link
Contributor

Choose a reason for hiding this comment

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

I agree with this, for some use cases generics are basically required though (e.g., for our self description using built-in blocks). But we do not have to expose that much of it to users. We should aim for empirically verifying that our hypotheses are correct though, yeah.


```jayvee
valuetype CorrelationCoefficient {
attributes: [
Copy link
Member

@georg-schwarz georg-schwarz Jul 31, 2023

Choose a reason for hiding this comment

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

Question on how to name this concept: attributes vs. properties?

Copy link
Member Author

Choose a reason for hiding this comment

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

No strong feelings. I used attributes because that's what I know it by. Let me ask ChatGPT for any subtle semantic differences between attribute and property. Try this: "Hi! Is there any significant semantic difference between the words attribute and property when describing something?"

Copy link
Contributor

Choose a reason for hiding this comment

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

(I'd argue for following the existing naming of properties here as well with no good case for the other direction).

value oftype decimal;
];
constraints: [
MinusOneToPlusOneRange; // Don't know how to attach this to value attribute
Copy link
Member

@georg-schwarz georg-schwarz Jul 31, 2023

Choose a reason for hiding this comment

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

If we refactor the constraints, the constraints could always refer to the complete valuetype (atomic or composite). This would also allow constraints spanning different attributes of a valuetype.

Probably a bad example with the alternative flat syntax I proposed in another comment:

valuetype SumIsOne {
    attribute x1 oftype decimal;
    attribute x2 oftype decimal;

    constraint sumIsOne: x1 + x2 == 1.0;
    constraint x1isPositive: x1 > 0;
    constraint x2isPositive: x2 > 0;
}

Copy link
Member

Choose a reason for hiding this comment

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

Alternative, with the current constraints, we could introduce sth like this on keyword:

valuetype CorrelationCoefficient {
    attributes: [
        value oftype decimal,
    ];
    constraints: [
        MinusOneToPlusOneRange on value,
    ]
}

Copy link
Contributor

Choose a reason for hiding this comment

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

I think the first flat syntax is nice, it could work with references constraints by instantiating them and assigning the values from attributes from them.

Consider:

constraint IsPositive on integer: value >= 0;

valuetype SumIsOne {
    attribute x1 oftype decimal;
    attribute x2 oftype decimal;

    constraint sumIsOne: x1 + x2 == 1.0;
    constraint x1isPositive: IsPositive(value = x1);
    constraint x2isPositive: IsPositive(value = x2);
}

This is a pretty big change because it introduces syntax for instantiating a constraint and assigning values to it but I'd like to throw it out there for discussion. Some syntax alternatives for that are possible of course, but I also like explicit parameter naming.

Copy link
Member Author

Choose a reason for hiding this comment

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

+1 on "flat syntax" over collections (keyword in plural form) if we can have only one.

On Philip's example I observe a subtle syntactic incongruence I think which is the use of : for constraint but not attribute.

Copy link
Contributor

Choose a reason for hiding this comment

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

We had this discussion before I think and it is true. My intuition is that we describe a property of the SumIsOne valuetype with both (attribute x1 oftype decimal is a property that is of type decimal attribute with the name x1, constraint sumIsOne is a property of type constraint with the name sumIsOne). The syntactic difference then comes from assigning a value to one (the constraint has an expression assigned) and assigning no value to the attribute, just declaring it's existence.


## Alternatives

Conceivably, we could use multiple inheritance for multi-attribute value types... just joking.
Copy link
Member

Choose a reason for hiding this comment

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

But how about inheritance in general?

Right now, this syntax does not allow an extension of an existing valuetype. I would see value offering it, though:

valuetype PositiveCorrelationCoefficient extends CorrelationCoefficient {
    // inherits all attributes
    // could add further attributes?

    // inherits all constraints
    // could ad further constraints
    constraints: [
         ZeroToPlusOneRange,
    ]
}

Copy link
Contributor

Choose a reason for hiding this comment

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

I'd vote for keeping inheritance out of this RFC to have a smaller scope :).

Copy link
Member Author

Choose a reason for hiding this comment

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

I'm all for single inheritance but agree that it is not part of this RFC.

@rhazn
Copy link
Contributor

rhazn commented Jul 31, 2023

Love it!

I think we should consider adapting the constraints syntax to be better suited for the extension to multi-attribute valuetypes (see the comments).

Also my thoughts, left some comments as well but I think this is on a good path 👍

@rhazn
Copy link
Contributor

rhazn commented Aug 1, 2023

I think we have a very similar vision here, our normal flow would be for the RFC creator (in this case you @dirkriehle) to update their RFC with the feedback as the understand it, push it to this PR and retrigger a review.

If you have the time you can follow that workflow, otherwise we'd bring this to a future meeting (e.g., thursday) and can sync in person.

@dirkriehle dirkriehle merged commit 962f456 into jvalue:main Jan 22, 2024
2 checks passed
@github-actions github-actions bot locked and limited conversation to collaborators Jan 22, 2024
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants