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.

Sign up for GitHub

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

Jump to bottom

added: isArrayOf rule [N4S] #488 #499

Merged
merged 9 commits into from
Nov 15, 2020
Merged

added: isArrayOf rule [N4S] #488 #499

Show file tree
Hide file tree
Changes from 4 commits
Commits
Show all changes
9 commits
Select commit Hold shift + click to select a range
f557563
add rule js
Moses3301 Nov 12, 2020
b2ffc97
Add test for isArrayOf rule
Moses3301 Nov 13, 2020
cd5771c
Merge branch 'latest' into add_arrayOf_rule
Moses3301 Nov 13, 2020
ad85801
change back the test script
Moses3301 Nov 13, 2020
974a009
Merge branch 'latest' into add_arrayOf_rule
Moses3301 Nov 14, 2020
0859bf8
Support the new lazy rules
Moses3301 Nov 14, 2020
33ab442
Add tests #488
Moses3301 Nov 14, 2020
d17d07b
add more test (#488)
Moses3301 Nov 14, 2020
1381890
Merge branch 'latest' into add_arrayOf_rule
Moses3301 Nov 14, 2020
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Jump to
Jump to file
Failed to load files.
Diff view
Diff view
3 changes: 3 additions & 0 deletions jsconfig.json
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -71,6 +71,9 @@
"isArray": [
"./packages/n4s/src/rules/isArray.js"
],
"isArrayOf": [
"./packages/n4s/src/rules/isArrayOf.js"
],
"isBetween": [
"./packages/n4s/src/rules/isBetween.js"
],
Expand Down
2 changes: 1 addition & 1 deletion package.json
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -6,7 +6,7 @@
"prettier-watch": "onchange '**/*.js' '**/*.json' -- prettier --write {{changed}}",
"dev": "onchange -i './packages/**/src/**/*.js' -- yarn genJSConfig",
"genJSConfig": "node ./scripts/genJsconfig",
"test": "jest --projects ./packages/*",
"test": "jest --projects ./packages/*",
Copy link
Owner

Choose a reason for hiding this comment

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

Suggested change
"test": "jest --projects ./packages/*",
"test": "jest --projects ./packages/*",

"lint": "eslint . --ignore-path .gitignore",
"pretest": "yarn genJSConfig"
},
Expand Down
40 changes: 40 additions & 0 deletions packages/n4s/src/rules/__tests__/isArrayOf.test.js
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -0,0 +1,40 @@
import { isArrayOf } from 'isArrayOf';
import enforse from 'enforce'
Copy link
Owner

Choose a reason for hiding this comment

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

typo 🙈

Suggested change
import enforse from 'enforce'
import enforce from 'enforce'


describe('Tests isArrayOf rule', () => {

it('Should return true if all elements are ture for one or more rules', () => {
expect(isArrayOf([3,4,5,'six',7],
enforse.greaterThan(2),
enforse.isString())
).toBe(true);
});


it('Should return false if one element or more fails all rules', () => {
expect(isArrayOf([3,4,5,['s','i','x'],7],
enforse.greaterThan(2),
enforse.isString())
).toBe(false);
});


describe('Tests for recursive call', ()=>{

it('Should return true if all elements are ture for one or more rules', () => {
Copy link
Owner

Choose a reason for hiding this comment

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

typo 🙈

Suggested change
it('Should return true if all elements are ture for one or more rules', () => {
it('Should return true if all elements are true for one or more rules', () => {

also here:
https://github.com/ealush/vest/pull/499/files#diff-e2dfd7aa604cef61b1c17ca8cf0a9e4fc5bbe176d63350fbf4a01a6d95f68219R6

expect(isArrayOf([3,4,5,['s','i','x'],7],
enforse.greaterThan(2),
enforse.isArrayOf(enforse.isString()))
).toBe(true);
});

it('Should return false if one element or more fails all rules', () => {
expect(
isArrayOf([3,4,5,['s','i','x'],7],
enforse.greaterThan(2),
enforse.isArrayOf(enforse.isNumber()))
).toBe(false);
});
});

Copy link
Owner

@ealush ealush Nov 13, 2020 •
edited

Choose a reason for hiding this comment

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

I would also add one integration test that makes sure everything works together with enforce

Here's an example, should probably be more thorough.

describe('as part of enforce', () => {
  it('should return silently when valid', () => {
    enforce([1,2,'3']).isArrayOf(enforce.isNumber())
    enforce([1,2,'3']).isArrayOf(enforce.isString())
    enforce([1,2,'3']).isArrayOf(enforce.isNumeric(), enforce.lessThan(5).greaterThan(0))
  });

  it('should throw an exception when invalid', () => {
    expect(() => enforce([1,2,'3']).isArrayOf(enforce.isNull())).toThrow()
    expect(() => enforce([1,2,'3']).isArrayOf(enforce.isNumber(), enforce.greaterThan(5))).toThrow()
  });
})

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Thank you for the replay,
I'm working on it, and I have a question
in the 'as part of enforce' -> 'should return silently when valid' test is

enforce([1,2,'3']).isArrayOf(enforce.isNumber())
enforce([1,2,'3']).isArrayOf(enforce.isString())

need to return silently? '3' isn't a number and '1','2' isn't a string.
did you meant to write
enforce([1,2,'3']).isArrayOf(enforce.isNumber(), enforce.isString())?

Copy link
Owner

Choose a reason for hiding this comment

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

Ahh, right. You are correct. I just wrote it as an example. Should be more similar to what you wrote.

});
4 changes: 4 additions & 0 deletions packages/n4s/src/rules/isArrayOf.js
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -0,0 +1,4 @@
export function isArrayOf(value, ...rules) {
if (value.some(element => !rules.some(fn => fn(element)))) {return false};
return true;
};
Copy link
Owner

Choose a reason for hiding this comment

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

There are a couple of things I would consider changing in this function.

  1. value might be undefined, and in some cases that would be ok. I would add an initial isArray safeguard at the beginning, and if value is not an array, return false immediately. At the moment, this function would throw.

  2. I think you could skip the negation (!) on !rules.some if you switch value.some with value.every.

  3. The inner function fn => fn(element) wouldn't work anymore due to a recent change I made to the lazy rules. They now support chaining as well, and to run them you need to call .test. You could use the runLazyRules(fn, element)

Suggested change
export function isArrayOf(value, ...rules) {
if (value.some(element => !rules.some(fn => fn(element)))) {return false};
return true;
};
import runLazyRules from 'runLazyRules';
export function isArrayOf(value, ...rules) {
return !value.every(element => rules.some(ruleChain => runLazyRules(ruleChain, element)))
};

2 changes: 2 additions & 0 deletions packages/n4s/src/rules/rules.js
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -27,6 +27,7 @@ import { numberEquals, numberNotEquals } from 'numberEquals';
import { shorterThan } from 'shorterThan';
import { shorterThanOrEquals } from 'shorterThanOrEquals';
import { startsWith, doesNotStartWith } from 'startsWith';
import { isArrayOf } from 'isArrayOf';
Copy link
Owner

Choose a reason for hiding this comment

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

I think this file export would be more suitable with compounds rather than rules, so instead of importing and exporting it here, it would probably make more sense to do it here:

https://github.com/ealush/vest/blob/latest/packages/n4s/src/enforce/compounds/compounds.js


export default function rules() {
return {
Expand Down Expand Up @@ -82,5 +83,6 @@ export default function rules() {
shorterThan,
shorterThanOrEquals,
startsWith,
isArrayOf,
Copy link
Owner

Choose a reason for hiding this comment

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

Now that you added it to compounds, there's no need to have it here.

};
}