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

add core validator generator for Object-type props #8878

Conversation

hamzamunaf
Copy link
Contributor

@hamzamunaf hamzamunaf commented Dec 8, 2021

Summary

Adding Core Validator Generator for Object Type Props

References

Closes #8640

Reviewer guidance

Use the ObjectPropValidator function like this:

		var users = {

			"props": {

				"user": {
					"name": "Hamza Munaf",
					"age": 24
				}

			}

		};

		//
		objectPropValidator( { props: {
				user: {
					type: Object,
					required: true,
					validator: objectPropValidator({
						name: {
							type: String,
							required: true,
						},
						age: {
							type: Number,
							required: true,
							validator(value) {
								return value > 0;
							},
						},
					}),
				},
			}
		} )(users);


Testing checklist

  • Contributor has fully tested the PR manually
  • If there are any front-end changes, before/after screenshots are included
  • Critical user journeys are covered by Gherkin stories
  • Critical and brittle code paths are covered by unit tests

PR process

  • PR has the correct target branch and milestone
  • PR has 'needs review' or 'work-in-progress' label
  • If PR is ready for review, a reviewer has been added. (Don't use 'Assignees')
  • If this is an important user-facing change, PR or related issue has a 'changelog' label
  • If this includes an internal dependency change, a link to the diff is provided

Reviewer checklist

  • Automated test coverage is satisfactory
  • PR is fully functional
  • PR has been tested for accessibility regressions
  • External dependency files were updated if necessary (yarn and pip)
  • Documentation is updated
  • Contributor is in AUTHORS.md

Copy link
Member

@rtibbles rtibbles left a comment

Choose a reason for hiding this comment

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

A good start, a test suite would help to demonstrate that the validator is working as intended. Some cleanup and simplification can be done too.

@@ -25,3 +25,195 @@ export function validateLearningActivity(arr) {
const isValidLearningActivity = v => Object.values(LearningActivities).includes(v);
return arr.length > 0 && arr.every(isValidLearningActivity);
}
export function objectPropValidator( options ){
Copy link
Member

Choose a reason for hiding this comment

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

Would be good to have a test suite to demonstrate the validation happening here.

} catch {}
return match ? match[1] : (rawType.length > 0 ? rawType : ((typeof variable == "function" || typeof variable.name != "undefined") ? variable.name : ''));
};
var _isUndefined = function(variable) {
Copy link
Member

Choose a reason for hiding this comment

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

I would suggest using lodash helper functions here e.g. https://lodash.com/docs/4.17.15#isBoolean instead of defining your own functions here.

var _isUndefined = function(variable) {
return (typeof variable === "undefined");
};
var _isSet = function(variable) {
Copy link
Member

Choose a reason for hiding this comment

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

This function name is confusing, as Set is an ES6 native type.

@@ -25,3 +25,195 @@ export function validateLearningActivity(arr) {
const isValidLearningActivity = v => Object.values(LearningActivities).includes(v);
return arr.length > 0 && arr.every(isValidLearningActivity);
}
export function objectPropValidator( options ){
var args = arguments || [];
Copy link
Member

Choose a reason for hiding this comment

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

Preferable to use ES6 const and let declarations.

@@ -25,3 +25,195 @@ export function validateLearningActivity(arr) {
const isValidLearningActivity = v => Object.values(LearningActivities).includes(v);
return arr.length > 0 && arr.every(isValidLearningActivity);
}
export function objectPropValidator( options ){
Copy link
Member

Choose a reason for hiding this comment

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

I think overall this could be simplified and cleaned up with a type map:

import isNumber from 'lodash/isNumber';
import isBoolean from 'lodash/isBoolean';

const typeValidatorMap = {
  Number: isNumber,
  Boolean: isBoolean,
}

This is an example, but could be extended to all supported types.

Then when going to check can just do:

typeValidatorMap[option.type](data);

@indirectlylit
Copy link
Contributor

Minor request: could we please rename objectPropValidator to just objectValidator? I have a use-case for this which is not a prop.

@indirectlylit
Copy link
Contributor

also, please check the linting and formatting:

https://kolibri-dev.readthedocs.io/en/develop/getting_started.html#linting-and-auto-formatting

@indirectlylit
Copy link
Contributor

indirectlylit commented Dec 13, 2021

Hi @hamzamunaf – thank you very much for your work!

I wanted to move forward with using the results in some ongoing refactoring I'm doing, so I took your commit and integrated it into a new PR here along with the above feedback from @rtibbles: #8902, so I'll close this as superseded. Your review of the new PR would be appreciated.

I've also opened a new follow-up issue here: #8903 – if you're interested in continuing, your help there would be appreciated!

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.

add core validator generator for Object-type props
3 participants