Skip to content
This repository has been archived by the owner on Feb 18, 2024. It is now read-only.

feat: rule test supports function #172

Merged
merged 1 commit into from
May 17, 2019

Conversation

tinymins
Copy link
Contributor

@tinymins tinymins commented May 7, 2019

Fix #171.

@tinymins tinymins force-pushed the rule-test-function branch from e968f35 to c458681 Compare May 7, 2019 09:13
Copy link
Member

@eliperelman eliperelman left a comment

Choose a reason for hiding this comment

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

Looks good, just have a couple nits.

test/Rule.js Outdated
@@ -131,6 +131,15 @@ test('toConfig with values', t => {
});
});

test('toConfig with test function', t => {
const rule = new Rule();
const test = s => s.indexOf('.js') >= 0;
Copy link
Member

Choose a reason for hiding this comment

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

Can you make this:

const test = s => s.includes('.js');

src/Rule.js Outdated
@@ -87,7 +87,11 @@ const Rule = Orderable(
}

if (!omit.includes('test') && 'test' in obj) {
this.test(obj.test instanceof RegExp ? obj.test : new RegExp(obj.test));
this.test(
obj.test instanceof RegExp || obj.test instanceof Function
Copy link
Member

Choose a reason for hiding this comment

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

Can you change obj.test instanceof Function to typeof obj.test === 'function'?

@tinymins tinymins force-pushed the rule-test-function branch from c458681 to 5e23771 Compare May 17, 2019 10:22
@tinymins tinymins force-pushed the rule-test-function branch from 5e23771 to 3f1b951 Compare May 17, 2019 10:24
@tinymins
Copy link
Contributor Author

@eliperelman Thanks for your advice, all changes applied.

@edmorley edmorley requested a review from eliperelman May 17, 2019 14:14
Copy link
Member

@eliperelman eliperelman left a comment

Choose a reason for hiding this comment

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

Looks great, thank you so much!

@eliperelman eliperelman merged commit 6c2b9ef into neutrinojs:master May 17, 2019
@edmorley
Copy link
Member

Published in v6.1.0 :-)

Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Development

Successfully merging this pull request may close these issues.

Rule test cannot be a function?
3 participants