Issue#50 validation rules #52

crsanti · 2017-03-17T13:24:41Z

There was implemented next rules:

required: Returns true given true value (for checkboxes) and non-empty string (trimed by default, can be changed with { customParams: { trim: false } }. Returns false for falsy values.
minLength: Returns true if given value is string and its length is lesser or equals that given length option in { customParams: { length: <number> } }. null and undefined return true (intended behaviour to treat them as uninitialized values). Returns false if mentioned conditions are not met.
maxLength: Returns true if given value is string and its length is greater or equals that given length option in { customParams: { length: <number> } }. null and undefined return true (intended behaviour to treat them as uninitialized values). Returns false if mentioned conditions are not met.
email: Returns true if value is a string and matches the RegExp. Otherwise, it returns false.
pattern: Returns true if given value matches with the described pattern in { customParams: { pattern: <RegExp | string> } }. If given pattern is a string it will be treated as a full RegExp (NEED REVISION HERE).

Exported validationFunctions in index.ts

Renamed constants in rules. Removed customParams in email. Updated samples.

nasdan · 2017-03-20T08:48:43Z

lib/lcformvalidation.d.ts

+  function pattern(value: any, vm: any, customParams: PatternParams): FieldValidationResult;
+}
+
+


Remove extra space

nasdan · 2017-03-20T08:53:39Z

lib/src/rules/index.ts

+  minLength,
+  maxLength,
+  email,
+  pattern


Add trailing comma

nasdan · 2017-03-20T08:56:05Z

lib/src/rules/length.ts

+}
+
+export function parseLengthParams(customParams: LengthParams, errorMessage: string) {
+  const length = customParams.length === null ? NaN : Number(customParams.length);


What happends if customParams.length is undefined?

Why do you have to parse customParams.length to number if LengthParams.lenght field is a number?

It is declared as a number in TypeScript but when it is transpiled there is no type checking in runtime.

If it is undefined it is casted with Number() and getting NaN. I asked for null and did not included undefined because when you do a Number(null) you get 0 instead of NaN and if you do Number(undefined) you get NaN.

nasdan · 2017-03-20T09:27:31Z

lib/src/rules/length.ts

+  return length;
+}
+
+interface LengthValidatorFunction {


Where are you using this interface?

I'll remove it

nasdan · 2017-03-20T09:28:34Z

lib/src/rules/length.ts

+  }
+
+  // Don't validate non string values
+  return true;


Why don't use ternary?

nasdan · 2017-03-20T09:36:02Z

lib/src/rules/maxLength.ts

+}
+
+function isStringLengthValid(value: string, length: number) {
+  return isNaN(length) ? false : value.length <= length;


It's necessary check that length isNaN?

What about check value is != null or undefined?

To improve readability is better introduce a carriage return after ? and :

It's necessary check that length isNaN?
Yes. Length must be a number.

What about check value is != null or undefined?
isNaN does an implicit Number(<value>).

To improve readability is better introduce a carriage return after ? and :.
Do you mean insert (CR)/LF after return statement like this?:

function isStringLengthValid(value: string, length: number) { return isNaN(length) ? false : value.length <= length; }

That has no sense to me.

No I said something like:

function isStringLengthValid(value: string, length: number) { return isNaN(length) ? false : value.length <= length; }

Btw to improve readability could be write this code like:

function isStringLengthValid(value: string, length: number) { return !isNaN(length) && value.length <= length; }

nasdan · 2017-03-20T09:40:49Z

lib/src/rules/maxLength.ts

+  const validationResult = new FieldValidationResult();
+
+  validationResult.succeeded = isValid;
+  validationResult.key = 'MAX_LENGTH';


Typo: validationResult.type = 'MAX_LENGTH';

Yes, I'll correct it and its unit tests.

nasdan · 2017-03-20T09:41:30Z

lib/src/rules/minLength.ts

+  const validationResult = new FieldValidationResult();
+  validationResult.errorMessage = isValid ? '' : `The value provided must have at least ${length} characters.`;
+  validationResult.succeeded = isValid;
+  validationResult.key = 'MIN_LENGTH';


Typo: validationResult.type = 'MIN_LENGTH';

nasdan · 2017-03-20T09:42:42Z

lib/src/rules/minLength.ts

+}
+
+function isStringLengthValid(value: string, length: number) {
+  return value.length >= length;


You have to check is value != null or undefined before access length property

This method is called before isLengthValid that already checks if value is a string.

nasdan · 2017-03-20T09:44:17Z

lib/src/rules/pattern.ts

+  pattern: string | RegExp;
+}
+
+const BAD_PARAMETER = 'FieldValidationError: pattern option for pattern validation is mandatory. Example: { pattern: /\d+/ }.';


Add carriage return afer this const like other rules

nasdan · 2017-03-20T09:45:43Z

lib/src/rules/pattern.ts

+}
+
+function parsePattern({ pattern }: PatternParams): RegExp {
+  if (pattern === undefined || pattern === null) {


Is this condition equals to:

if (!pattern) { .... }

That's not true for "", false or 0 values. I tried to avoid RegExp like /null/ I'll change that to:

if (typeof pattern === 'boolean' || pattern === null) { throw new Error(BAD_PARAMETER); } return getRegExp(pattern);

to avoid /true/ and /false/ too (with a comment).

nasdan · 2017-03-20T09:51:28Z

lib/src/rules/pattern.ts

+
+function getRegExp(pattern) {
+  if (typeof pattern === 'string') {
+    return new RegExp(`^${pattern}$`);


Why do you have to force to ^...$? User could pass a pattern like:

{ pattern: 'abc$' }

We can add an flag in customParams to enable this feature or disable it. We need discussion about this behavior.

nasdan · 2017-03-20T09:53:21Z

lib/src/rules/pattern.ts

+  if (pattern instanceof RegExp) {
+    return pattern;
+  }
+  return new RegExp(pattern);


Which is this case?

User is allowed to enter a RegExp or string to use the pattern validator.

nasdan · 2017-03-20T09:56:35Z

lib/src/rules/required.ts

+  const validationResult = new FieldValidationResult();
+  const isValid = isValidField(value, Boolean(customParams.trim));
+  if (!isValid) {
+    validationResult.errorMessage = 'Please fill in this mandatory field.';


Why don't use ternary as other rules?

validationResult.errorMessage = isValid? '' : 'Please fill in this mandatory field.';

I'll change it.

nasdan · 2017-03-20T10:01:51Z

lib/src/rules/required.ts

+
+function isValidField(value, trim: boolean): boolean {
+  if (typeof value === 'string') {
+    return isStringValid(value, trim);


Please avoid multiples returns. You can write this code like:

function isValidField(value, trim: boolean): boolean { return isInitialized(value) && isString(value) && isStringValid(value, trim); }

nasdan · 2017-03-20T15:27:13Z

lib/src/rules/length.ts

+  length: number,
+  validatorFn: (value: string, length: number) => boolean
+): boolean {
+  return typeof value === 'string' ? validatorFn(value, length) : true;


return typeof value === 'string' ?
validatorFn(value, length) :
true;

nasdan · 2017-03-20T15:52:08Z

lib/src/rules/spec/email.spec.ts

+
+    it('should return false if value is undefined', () => {
+      // Arrange
+      const value = null;


Typo: const value = undefined?

Added typings on returned values by some functions.

crsanti added 8 commits March 17, 2017 11:36

Added required rule

ef1993a

Added minlength rule

4b16b51

Added maxlength rule

aba7c39

Added email validation rule

3700149

Added pattern validation rule.

1f428f5

Renamed requiredField to required.

9c93ac3

Exported validationFunctions in index.ts

Merge branch 'master' into issue#50-validation-rules

8288855

Refactored min and max length rules.

c865825

Renamed constants in rules. Removed customParams in email. Updated samples.

crsanti added the implementation label Mar 17, 2017

crsanti self-assigned this Mar 17, 2017

crsanti requested review from JaimeSalas, brauliodiez and nasdan March 17, 2017 13:24

nasdan reviewed Mar 20, 2017

View reviewed changes

lib/src/rules/index.ts Outdated

minLength,

maxLength,

email,

pattern

Copy link

Member

nasdan Mar 20, 2017

Choose a reason for hiding this comment

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

Add trailing comma

nasdan reviewed Mar 20, 2017

View reviewed changes

lib/src/rules/length.ts Outdated

}

// Don't validate non string values

return true;

Copy link

Member

nasdan Mar 20, 2017

Choose a reason for hiding this comment

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

Why don't use ternary?

nasdan reviewed Mar 20, 2017

View reviewed changes

crsanti added 2 commits March 20, 2017 11:51

Updated changes based on comments in PR.

9d41ea0

Removed unused interface.

44e92c0

nasdan reviewed Mar 20, 2017

View reviewed changes

nasdan approved these changes Mar 20, 2017

View reviewed changes

crsanti added 2 commits March 21, 2017 09:22

Reformatted some ternary operators.

c7c1e5a

Allowed email to be empty.

2b7ede8

Added typings on returned values by some functions.

crsanti merged commit e82a6c7 into master Mar 21, 2017

crsanti deleted the issue#50-validation-rules branch March 21, 2017 08:57

crsanti mentioned this pull request Mar 31, 2017

Implement some basic validators in the core #50

Closed

		function pattern(value: any, vm: any, customParams: PatternParams): FieldValidationResult;
		}

Issue#50 validation rules #52

Issue#50 validation rules #52

Uh oh!

Conversation

crsanti commented Mar 17, 2017

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

crsanti Mar 20, 2017 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

crsanti Mar 20, 2017 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

crsanti Mar 20, 2017 •

edited

Loading

crsanti Mar 20, 2017 •

edited

Loading