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

rules: core: Add minValueInclusive rule #389

Merged
merged 3 commits into from
Mar 29, 2021
Merged

Conversation

Lathrisk
Copy link

#378

Adds a rule to check the values against minValueInclusive when it is supplied.

@Lathrisk Lathrisk self-assigned this Mar 26, 2021
Copy link
Contributor

@nickevansuk nickevansuk left a comment

Choose a reason for hiding this comment

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

Approved assuming minor changes

tests: {
belowMinimum: {
description: 'Raises a failure if the value is below the associated minValueInclusive property.',
message: 'The value of this property should be above or equal to {{minValueInclusive}}.',
Copy link
Contributor

Choose a reason for hiding this comment

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

Suggest slightly more conventional language here around "greater than".

Suggested change
message: 'The value of this property should be above or equal to {{minValueInclusive}}.',
message: 'The value of this property must be greater than or equal to {{minValueInclusive}}.',

Also in general worth noting that within messages anything that results in a FAILURE must use the language "must", and anything that results in a WARNING must use the language "should". This is because "must" and "should" key words actually have defined interpretations (https://openactive.io/open-booking-api/EditorsDraft/1.0CR3/#conformance)

Comment on lines +64 to +65
-100.1,
-100,
Copy link
Contributor

Choose a reason for hiding this comment

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

Is best practice to test boundary conditions, so e.g.

Suggested change
-100.1,
-100,
-100.1,
-100,
-3.9,

Comment on lines +29 to +30
89.12345,
89,
Copy link
Contributor

Choose a reason for hiding this comment

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

Is best practice to test boundary conditions, so e.g.

Suggested change
89.12345,
89,
89.12345,
89,
4.1,

@Lathrisk Lathrisk merged commit 6bfedea into master Mar 29, 2021
@Lathrisk Lathrisk deleted the 378/minvalueinclusive branch March 29, 2021 13:31
@nickevansuk
Copy link
Contributor

@Lathrisk sorry just spotted this, should be:

The value of this property must be greater than

anything that results in a FAILURE must use the language "must", and anything that results in a WARNING must use the language "should". This is because "must" and "should" key words actually have defined interpretations (https://openactive.io/open-booking-api/EditorsDraft/1.0CR3/#conformance)

@Lathrisk
Copy link
Author

I'll put together an update shortly!

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