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

Incorporate missing change proposals to RFC 00014 #595

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

Conversation

georg-schwarz
Copy link
Member

I think we didn't integrate some change proposals we agreed on. This PR introduces those changes.

];
property correlation oftype decimal;

constraint minusOneToPlusOneRange: MinusOneToPlusOneRange(value = correlation);
Copy link
Member Author

Choose a reason for hiding this comment

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

Not entirely sure if we really agreed on the syntax to call a constraint on a property

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 did (and I don't think we have a constraint keyword somewhere that is not a constraint definition, right? I'd keep the suggestions in this PR more minimal with

constraints: [
  MinusOneToPlusOneRange on correlation
]

I think that nicely aligns with our current syntax style (so doesn't introduce new concepts like constructor-like brackets and assignments). We will of course have to evolve on that (e.g. constraints over multiple attributes, AND/ORing constraints etc) but I'd do that in a later RFC.

Copy link
Member Author

Choose a reason for hiding this comment

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

How about the following?

constraint MinusOneToPlusOneRange on correlation;

I'd really like to get rid of the array syntax and more closely align with the "flat over nested" design principle.

Copy link
Contributor

Choose a reason for hiding this comment

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

You mean similar to the property syntax of just listing all the properties in that way? I am good with it, happy with your suggestion there.

Copy link
Member Author

Choose a reason for hiding this comment

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

Yes exactly. I feel like, currently, we have two different approaches to defining lists. And I'd like to introduce more consistency by also using the same flat syntax as for property

1. more similar to traditional approaches and
2. is needed anyway for multi-attribute value types.

Thus, this RFC removes the `oftype` keyword in a value type definitions and proposes an attribute-based syntax that is extendable to a future multi-attribute syntax.
Copy link
Contributor

Choose a reason for hiding this comment

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

Suggested change
Thus, this RFC removes the `oftype` keyword in a value type definitions and proposes an attribute-based syntax that is extendable to a future multi-attribute syntax.
Thus, this RFC removes the reference to a parent value type in a value type definition and proposes an explicit attribute-based syntax instead that is extendable to a future multi-attribute syntax.

];
property correlation oftype decimal;

constraint minusOneToPlusOneRange: MinusOneToPlusOneRange(value = correlation);
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 did (and I don't think we have a constraint keyword somewhere that is not a constraint definition, right? I'd keep the suggestions in this PR more minimal with

constraints: [
  MinusOneToPlusOneRange on correlation
]

I think that nicely aligns with our current syntax style (so doesn't introduce new concepts like constructor-like brackets and assignments). We will of course have to evolve on that (e.g. constraints over multiple attributes, AND/ORing constraints etc) but I'd do that in a later RFC.

```

While more verbose, it prepares the way for
The syntax is kept flat rather than using arrays.
Copy link
Contributor

Choose a reason for hiding this comment

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

Suggested change
The syntax is kept flat rather than using arrays.
The syntax is kept flat rather than using arrays in alignment with the [Jayvee Design Principles](https://jvalue.github.io/jayvee/docs/0.4.0/dev/design-principles#jayvee-manifesto).

Copy link
Contributor

Choose a reason for hiding this comment

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

This sentence does not make sense anymore if we apply @rhazn's suggestion above.

While more verbose, it prepares the way for
The syntax is kept flat rather than using arrays.

Despite referencing reusable constraints, the syntax also allows in-place constraint definition:
Copy link
Contributor

Choose a reason for hiding this comment

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

I would keep this out of the RFC to make it smaller. We should probably have in-place constraint definitions but imho can do so in a follow-up.

Copy link
Contributor

Choose a reason for hiding this comment

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

I wouldn't mind adding it here. It's not huge anyway.

Copy link
Contributor

Choose a reason for hiding this comment

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

Just for a view on the syntax, if we go with my suggestion, that would be:

constraints: [ value >= 1 and value <=1 on correlation ]

equivalent to

constraints: [ MinusOneToPlusOneRange on correlation ]

My issue with that was that reading the in-place constraint definition with the following "on" is kind of confusing... But honestly, seeing how it looks here I am kind of happy with it. So I'll revert what I said and say we should have in place constraints as well, yeah.

Copy link
Contributor

Choose a reason for hiding this comment

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

Why shouldn't we use

constraints: [ correlation >= 1 and correlation <=1 ]

?

And combined:

constraints: [
    correlation >= 1 and correlation <=1,
    MinusOneToPlusOneRange on correlation
]

I don't see why we need to use on for in-place constraints as well

Copy link
Contributor

Choose a reason for hiding this comment

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

How would you model two different attributes in that case? The reason I put the on correlation at the end of the reference is to allow for more than one attribute, e.g. correlation and unit and you could have

constraints: [
    value >= 1 and value <=1 on correlation,
    SomethingElse on unit
]

I'd semi-strongly feel about continuing to use value for inside the constraint and a clear reference to which attribute it is on to allow for future multi-attribute value types.

Copy link
Contributor

Choose a reason for hiding this comment

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

I use correlation instead of value.
This allows for example

constraints: [
    correlation >= 1 and otherUnit <=1
]

Copy link
Contributor

Choose a reason for hiding this comment

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

Ah, fair enough. I dislike that because it introduces different syntax for inline constraints vs. references and I like the ability to always just create a constraint reference by c&p the inline code. But that is something that reasonable people can disagree on :D.

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 see your point @rhazn, and I see value in keeping the syntax consistent independently if defined in-line or as stand-alone element.

What do you think about changing the constraint definition syntax in a similar way to keep it consistent? Instead of the value keyword, constraint definitions would refer to the value type's properties they are based on:

constraint CorrelationCoefficient on decimal:
  value >= 1 and value <= 1; // "value" is the property name of builtin value types (so nothing changes for them)
  
constraint PostiveCorrelationConstraint on CorrelationCoefficient:
  correlation > 0; // "correlation" is a property of the value type CorrelationCoefficient
  
// Future, for multi-dimensional value types
constraint Coordinate2DInLeftUpperQuadrantConstraint on Coordinate2D:
  x > 0 and y > 0;

An obvious downside is the circular dependency of value type and constraint that could be incentivized (or should be prevented by hinting to in-line constraints in such cases).

Copy link
Contributor

Choose a reason for hiding this comment

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

I rewrote this comment three times, not sure I can come to a strong opinion so in the end I would probably accept majority opinion, but:

I think that is unfortunate because it again introduces magic with the value name for builtin value types. It also still means it is weird to reuse constraints between inline definitions and general definitions. I would prefer if we keep value in general for one-dimensional value types.

I can not really think of a good way to model constrains on multi-dimensional value types that do not need to know the names of properties... For consistencies sake I would go with value.x. That does not resolve the issue of course, but it keeps the rule intact that value always refers to the underlying value of the value type.

Copy link
Member Author

@georg-schwarz georg-schwarz Aug 12, 2024

Choose a reason for hiding this comment

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

Ok, I see two alternatives:

Alternative 1: Inline constraints use property names

Inline constraints use property names, while referenceable constraints use the value keyword for uni-dimensional (built-in) value type.

This leads to different syntax for inline and referenceable constraints.
However, users can call their property of single-attribute value types "value," and we can promote that through our documentation. Then, there is no syntactic difference when extracting an in-line constraint to a referenceable constraint.

// Referenceable constraint
valuetype CorrelationCoefficient {
  property correlation oftype decimal; 
  constraint minusOneToPlusOneRange: CorrelationCoefficient on correlation;
}
constraint CorrelationCoefficient on decimal:
  value >= -1 and value <= 1;

// Inline constraint (not following convention)
valuetype CorrelationCoefficient {
  property correlation oftype decimal; 

  constraint minusOneToPlusOneRange:
    correlation >= -1 and correlation <=1;
}

// Inline constraint (following convention)
valuetype CorrelationCoefficient {
  property value oftype decimal; // convention: should be called value for one-dim value types

  constraint minusOneToPlusOneRange:
    value >= -1 and value <=1;
}

Alternative 2: Constraints always use value

This alternative leads to similar syntax for inline and referenceable constraints.

// Referenceable constraint 
valuetype CorrelationCoefficient {
  property correlation oftype decimal; 
  constraint minusOneToPlusOneRange: CorrelationCoefficient on correlation;
}
constraint CorrelationCoefficient on decimal:
  value >= -1 and value <= 1;

// Inline constraint (not following convention)
valuetype CorrelationCoefficient {
  property correlation oftype decimal; 

  constraint minusOneToPlusOneRange on correlation:
    value >= -1 and value <=1;
}

The question remaining for this alternative is how to model multi-dimensional constraints, like @rhazn already mentioned... I tried a few times, but didn't come to a good solution.

Copy link
Contributor

@joluj joluj left a comment

Choose a reason for hiding this comment

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

I'm not really happy with the new syntax regarding the constraints. I like @rhazn's suggestion more, but I'm not really happy with that either.
But I think we will get better ideas when we use it a bit and when we think about AND, OR, XOR, ...

Since we need to go over the constraints later anyway, I'm fine with not finding an optimal solution now.

rfc/0014-attribute-based-valuetypes/README.md Outdated Show resolved Hide resolved
```

While more verbose, it prepares the way for
The syntax is kept flat rather than using arrays.
Copy link
Contributor

Choose a reason for hiding this comment

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

This sentence does not make sense anymore if we apply @rhazn's suggestion above.

While more verbose, it prepares the way for
The syntax is kept flat rather than using arrays.

Despite referencing reusable constraints, the syntax also allows in-place constraint definition:
Copy link
Contributor

Choose a reason for hiding this comment

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

I wouldn't mind adding it here. It's not huge anyway.

rfc/0014-attribute-based-valuetypes/README.md Outdated Show resolved Hide resolved
rfc/0014-attribute-based-valuetypes/README.md Show resolved Hide resolved
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