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

AJV $data support #920 #923

Merged
merged 11 commits into from
May 5, 2021
Merged

Conversation

ZakRabe
Copy link
Contributor

@ZakRabe ZakRabe commented May 5, 2021

Issue

Add support for the AJV $data option to Foal configuration. Resolves #920

Solution and steps

  • Add $data attribute to options passed to AJV initialization in getAjvInstance, defaulted to undefined
  • Updated documentation in "Validation and Sanitization" to list the new configuration property.
  • Added test to should accept custom configuration from the Config to test the case where $data is true. Tested both validation success and failure.
  • Added test to getAjvInstance to test the default case where $data is undefined.
  • Added test to check throwing a ConfigTypeError when settings.ajv.$data has an invalid type.

Checklist

  • update/check docs (code comments and docs/ folder).
  • update/check tests.
  • Update/check the cli generators.

@LoicPoullain LoicPoullain added this to Work In Progress in Issue tracking via automation May 5, 2021
@LoicPoullain LoicPoullain self-requested a review May 5, 2021 05:14
Copy link
Member

@LoicPoullain LoicPoullain left a comment

Choose a reason for hiding this comment

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

Thank you for taking care of this @ZakRabe and thank you for the style consistency of your tests. I added some notes below.

Also, to ensure that we have a stable product that is backwards compatible with previous versions, we should not add new defaults to the AJV instance configuration. Could you leave the default value $data as undefined? I guess this will require updating some of your tests.

@ZakRabe
Copy link
Contributor Author

ZakRabe commented May 5, 2021

@LoicPoullain Thanks for the feedback.

Updated getAjvInstance to remove the default, and mirrored this change in the docs.
Updated the tests to remove the silly deepStrictEqual mistake, and added the missing $ back to the errors object.
Updated the tests to check that AJV cannot validate $data references by default, and can use them when $data is true in the custom configuration.
Cleaned up extra spacing and resolved trailing comma differences between the rest of the code in the test and my own.

@LoicPoullain LoicPoullain changed the base branch from master to v2-4-0 May 5, 2021 12:40
@LoicPoullain LoicPoullain changed the base branch from v2-4-0 to master May 5, 2021 12:41
@LoicPoullain LoicPoullain changed the base branch from master to v2-4-0 May 5, 2021 12:41
@LoicPoullain LoicPoullain mentioned this pull request May 5, 2021
4 tasks
Copy link
Member

@LoicPoullain LoicPoullain left a comment

Choose a reason for hiding this comment

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

🚀

@LoicPoullain LoicPoullain merged commit e21f311 into FoalTS:v2-4-0 May 5, 2021
Issue tracking automation moved this from Work In Progress to Done / Closed This Release May 5, 2021
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Projects
Issue tracking
  
Done / Closed This Release
Development

Successfully merging this pull request may close these issues.

Request: Support $data on AJV configuration
2 participants