-
-
Notifications
You must be signed in to change notification settings - Fork 550
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
rational-numbers: Add ordering to spec #1736
Conversation
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This also raises the question: How are new optional test cases handled if they require an addition to the description?
I don't understand this. Addition to what description?
}, | ||
{ | ||
"uuid": "bd1eab16-7603-4a77-ae4e-23c930fe39cd", | ||
"description": "Ordering", |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I don't have a better alternative, but this description is fairly minimal. Maybe something like Rational numbers can be ordered
or something like that?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
The name is in the style of the other names, like "absolute value" or "multiplication". Do you think it should be longer anyway?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
There is some inconsistency in the test naming. I'm fine with keeping it as is therefore, but let's see what others think.
{ | ||
"uuid": "bd1eab16-7603-4a77-ae4e-23c930fe39cd", | ||
"description": "Ordering", | ||
"property": "<", |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
In the diffie-hellman exercise, we experimented with using a textual description of the property. This might be useful here too.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Interesting, didn't know about that exercise. Do you think that format would be better here? I feel like it'd need special handling in generators either way, so I'm not sure if a description like in diffie-hellman has an advantage? It would likely need the comment anyway for extra explanations.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
It would definitely need the comment. I do think a textual property might be somewhat easier to understand. What do you think @SleeplessByte?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Special handling is required either way (see linked-list); I think written out properties are more consistent with the other exercises.
Wouldn't care here 🤷🏽♂️🤗
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
(the app won't let me add a single comment)
The addition to the description.md. If one chooses not to implement this new test case, they'd have a pointless explanation in the description.md file regardless. Or am I missing some tooling that removes that? |
No we don't yet have a way to elegantly handle this. |
@SaschaMann Are you still interested in working on this PR? |
No but if anyone wants to pick it up, feel free. |
Should we have a "Available for pickup" label created for issues like these? @ErikSchierboom @iHiD |
This is a fun one, because I can't think of any sensible way to represent this in JSON. Therefore the test data is described in a comment. Testing only specific order operations would require adding dozens, possibly hundreds, of tests to cover all cases, so I don't think that's a feasible way of adding it to the canonical data.
An example implementation can be found on the Julia track.
This also raises the question: How are new optional test cases handled if they require an addition to the description?