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

feat(shared-data, api): remove 'default' from all '..ByVolume' properties #16878

Merged

Conversation

sanni-t
Copy link
Member

@sanni-t sanni-t commented Nov 18, 2024

Modifies the schema from ticket AUTH-832

Overview

After some discussion with product & hardware, we have decided to remove the 'default' keys from all '..ByVolume' properties in liquid classes since they were causing confusion and were trying to add a flexibility that would not be realistically needed.

The other change in this PR is changing the format of the schema for byVolume properties, turning them from a mapping of volume to value, to an array of 2-tuples containing the volume and corresponding value, removing the reliance on schema property name regex to ensure the volume is a valid float.

Test Plan and Hands on Testing

Added test to validate fixtures and definitions against schema

Changelog

  • removed 'default' entries from liquid class schema, definitions and fixtures, as well as from the PAPI liquid classes interface.
  • changed byVolume properties in schema from object to array of 2 element arrays

Review requests

Any place I've forgotten to update?

Risk assessment

None.

@sanni-t sanni-t requested review from a team as code owners November 18, 2024 22:13
@ddcc4
Copy link
Contributor

ddcc4 commented Nov 18, 2024

Hey, in liquid_class_definition.py, there were a bunch of things that were defined as dict[str, ...], I assume because we had to support default as a key in the dictionary.

With this change, can those definitions be tightened up to dict[float, ...] now? Or do they have to be dict[str, ...] because you want to use strings in the JSON dictionaries?

@jbleon95 jbleon95 requested a review from a team as a code owner November 19, 2024 16:23
@jbleon95 jbleon95 requested review from TamarZanzouri and removed request for a team November 19, 2024 16:23
@jbleon95
Copy link
Contributor

Hey, in liquid_class_definition.py, there were a bunch of things that were defined as dict[str, ...], I assume because we had to support default as a key in the dictionary.

With this change, can those definitions be tightened up to dict[float, ...] now? Or do they have to be dict[str, ...] because you want to use strings in the JSON dictionaries?

The latter, they should stay dict[str, ...] to represent the JSON keys

"required": ["default"]
"d+": {
"$ref": "#/definitions/positiveNumber"
}
Copy link
Contributor

Choose a reason for hiding this comment

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

Cool. So if I understand this properly:

You have to use d+ as the validator for the key because the keys are strings, and the most you can do to validate strings is to write a regex for it? But you can use #/definitions/positiveNumber as the validator for the values because those are numbers?

Are the keys limited to strictly d+? E.g., is 1.5 a valid value for the key, and would that match d+? (Would the decimal point match d+?)

},
{
"type": "number",
"minimum": 0.0
Copy link
Contributor

Choose a reason for hiding this comment

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

Is this autogenerated? Do you have to specify integers and numbers separately like this?

path.parse(fixturePath).base +
' ' +
JSON.stringify(validationErrors, null, 4)
)
Copy link
Contributor

Choose a reason for hiding this comment

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

FYI, I think you can just say console.log(a, b, c) instead of manually stringifying and concatenating them with +. In some environments (like the browser inspector or colorized consoles), it produces prettier output because the system can interpret each argument individually.

Similarly, if you don't need to stringify() the validationErrors, see if you can just log it as-is, again so that we can get pretty-printing in the console.

@jbleon95 jbleon95 merged commit fe29ce4 into edge Nov 21, 2024
73 checks passed
@andySigler andySigler deleted the liquid-classes-remove_default_keys_from_props_by_volume branch November 21, 2024 19:33
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