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

feat(isBefore): allow usage of options object #2088

Open
wants to merge 35 commits into
base: master
Choose a base branch
from
Open
Show file tree
Hide file tree
Changes from 28 commits
Commits
Show all changes
35 commits
Select commit Hold shift + click to select a range
5578dad
refactor: allow for splitting tests to different files
WikiRik Oct 18, 2022
afda0bd
feat(isAfter): allow usage of options object
WikiRik Oct 18, 2022
cad192e
style: make options italic
WikiRik Oct 18, 2022
5ba1a43
refactor: rename test file extension to .test.js
WikiRik Oct 19, 2022
71102d4
refactor: rename test-functions to testFunctions
WikiRik Oct 19, 2022
f32d228
refactor: implement suggestion from #2019 review
WikiRik Oct 23, 2022
7799d9b
refactor: remove custom repeat to use native function
WikiRik Oct 23, 2022
5375f17
refactor: implement suggestion new Date
WikiRik Oct 23, 2022
ae7a8a6
Refactor isBefore with options API
pixelbucket-dev Oct 25, 2022
f5b3068
Refactor isBefore tests
pixelbucket-dev Oct 25, 2022
4601b83
Refactor to simplify logic
pixelbucket-dev Oct 25, 2022
50318a3
Update README
pixelbucket-dev Oct 25, 2022
d7efe27
Refactor logic
pixelbucket-dev Oct 25, 2022
d8caed7
Improve README formatting
pixelbucket-dev Oct 25, 2022
65a9d71
Fix backwards-compat
pixelbucket-dev Oct 25, 2022
ded66eb
Remove redundant string assertion
pixelbucket-dev Oct 25, 2022
62c2e22
Fix comment
pixelbucket-dev Oct 25, 2022
e907cde
Reinstate legacy tests
pixelbucket-dev Oct 25, 2022
44eda79
Change arg name according to code review
pixelbucket-dev Jan 3, 2023
de17c53
Add line break according to code review
pixelbucket-dev Jan 3, 2023
c145be3
Revert change of simplifying toDate
pixelbucket-dev Jan 3, 2023
3cfab41
Fix whitespace issues
pixelbucket-dev Jan 3, 2023
15182ac
Fix test
pixelbucket-dev Jan 3, 2023
0557666
Merge branch 'master' into isBefore-options-refactor
pixelbucket-dev Feb 5, 2023
ab6b5ef
Fix tests
pixelbucket-dev Feb 5, 2023
0419de3
Format file for consistency with isBefore
pixelbucket-dev Feb 5, 2023
25f88c4
Remove redundant file
pixelbucket-dev Feb 5, 2023
6a5ec27
Remove old tests
pixelbucket-dev Feb 5, 2023
587b3ef
Add tests for undefined args
pixelbucket-dev Feb 5, 2023
05d07ea
Remove arguments: linter error
pixelbucket-dev Feb 5, 2023
daa1abd
Improve comment
pixelbucket-dev Feb 5, 2023
23478fd
Use recommended variable name
pixelbucket-dev Feb 5, 2023
84151bc
Improve readme text according to code review
pixelbucket-dev Feb 5, 2023
18d652b
Make isAfter arguments more robust
pixelbucket-dev Feb 5, 2023
57d8945
Split test cases into given and default end date
pixelbucket-dev Feb 6, 2023
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
2 changes: 1 addition & 1 deletion README.md
Original file line number Diff line number Diff line change
Expand Up @@ -97,7 +97,7 @@ Validator | Description
**isBase32(str [, options])** | check if the string is base32 encoded. `options` is optional and defaults to `{ crockford: false }`.<br/> When `crockford` is true it tests the given base32 encoded string using [Crockford's base32 alternative][Crockford Base32].
**isBase58(str)** | check if the string is base58 encoded.
**isBase64(str [, options])** | check if the string is base64 encoded. `options` is optional and defaults to `{ urlSafe: false }`<br/> when `urlSafe` is true it tests the given base64 encoded string is [url safe][Base64 URL Safe].
**isBefore(str [, date])** | check if the string is a date that is before the specified date.
**isBefore(str [, options])** | check if the string is a date that's before the specified date.<br/><br/>`options` is an object that defaults to `{ comparisonDate: Date().toString() }`.<br/><br/>**Options:**<br/>`comparisonDate`: Date to compare to. Defaults to the current date `Date().toString()` (now).
pixelbucket-dev marked this conversation as resolved.
Show resolved Hide resolved
**isBIC(str)** | check if the string is a BIC (Bank Identification Code) or SWIFT code.
**isBoolean(str [, options])** | check if the string is a boolean.<br/>`options` is an object which defaults to `{ loose: false }`. If `loose` is is set to false, the validator will strictly match ['true', 'false', '0', '1']. If `loose` is set to true, the validator will also match 'yes', 'no', and will match a valid boolean string of any case. (e.g.: ['true', 'True', 'TRUE']).
**isBtcAddress(str)** | check if the string is a valid BTC address.
Expand Down
1 change: 1 addition & 0 deletions src/lib/isAfter.js
Original file line number Diff line number Diff line change
Expand Up @@ -7,5 +7,6 @@ export default function isAfter(date, options) {

const comparison = toDate(comparisonDate);
const original = toDate(date);

return !!(original && comparison && original > comparison);
}
11 changes: 7 additions & 4 deletions src/lib/isBefore.js
Original file line number Diff line number Diff line change
@@ -1,9 +1,12 @@
import assertString from './util/assertString';
import toDate from './toDate';

export default function isBefore(str, date = String(new Date())) {
assertString(str);
const comparison = toDate(date);
export default function isBefore(str, options) {
pixelbucket-dev marked this conversation as resolved.
Show resolved Hide resolved
// accessing 'arguments' for backwards compatibility: isBefore(str [, comparisonDate])
// eslint-disable-next-line prefer-rest-params
const comparisonDate = (typeof options === 'object' ? options.comparisonDate : arguments[1]) || Date().toString();
pixelbucket-dev marked this conversation as resolved.
Show resolved Hide resolved
Copy link
Member

Choose a reason for hiding this comment

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

Suggested change
// accessing 'arguments' for backwards compatibility: isBefore(str [, comparisonDate])
// eslint-disable-next-line prefer-rest-params
const comparisonDate = (typeof options === 'object' ? options.comparisonDate : arguments[1]) || Date().toString();
// For backwards compatibility:
// isBefore(str [, date]), i.e. `options` could be used as argument for the legacy `date`
const comparisonDate = options?.comparisonDate || options || Date().toString();

Would this work as well? Copied from isAfter

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Nope, that would be a bug :(.

If comparisonDate is undefined, the ternary operator will therefore evaluate to options, which would be { comparisonDate: undefined }. The latter will break when using inside toDate, because it is not a string.

Well spotted anyway. This way I realised that I wanted to add tests to cover this scenario.

Copy link
Member

Choose a reason for hiding this comment

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

That's the case in isAfter as well right? So we can fix it for both validators in this PR I think

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Yes, that would be the case. I will add a fix.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I've fixed isAfter and added tests. FYI, the last test would fail with the old way in isAfter.


const comparison = toDate(comparisonDate);
const original = toDate(str);
pixelbucket-dev marked this conversation as resolved.
Show resolved Hide resolved

return !!(original && comparison && original < comparison);
}
35 changes: 0 additions & 35 deletions test/validators.test.js
Original file line number Diff line number Diff line change
Expand Up @@ -5103,41 +5103,6 @@ describe('Validators', () => {
});
});

it('should validate dates against an end date', () => {
test({
validator: 'isBefore',
args: ['08/04/2011'],
valid: ['2010-07-02', '2010-08-04', new Date(0).toString()],
invalid: ['08/04/2011', new Date(2011, 9, 10).toString()],
});
test({
validator: 'isBefore',
args: [new Date(2011, 7, 4).toString()],
valid: ['2010-07-02', '2010-08-04', new Date(0).toString()],
invalid: ['08/04/2011', new Date(2011, 9, 10).toString()],
});
test({
validator: 'isBefore',
valid: [
'2000-08-04',
new Date(0).toString(),
new Date(Date.now() - 86400000).toString(),
],
invalid: ['2100-07-02', new Date(2217, 10, 10).toString()],
});
test({
validator: 'isBefore',
args: ['2011-08-03'],
valid: ['1999-12-31'],
invalid: ['invalid date'],
});
test({
validator: 'isBefore',
args: ['invalid date'],
invalid: ['invalid date', '1999-12-31'],
});
});

it('should validate IBAN', () => {
test({
validator: 'isIBAN',
Expand Down
75 changes: 75 additions & 0 deletions test/validators/isBefore.test.js
Original file line number Diff line number Diff line change
@@ -0,0 +1,75 @@
import test from '../testFunctions';

describe('isBefore', () => {
it('should validate dates against an end date', () => {
pixelbucket-dev marked this conversation as resolved.
Show resolved Hide resolved
Copy link
Member

Choose a reason for hiding this comment

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

I didn't do it for isAfter but maybe we can split this up in two it blocks? One without arguments (or at least that don't use the old nor new syntax like []) and one with arguments. This way it's easier to see what the changes are of the two syntaxes and therefore the scenarios that we need to replicate
Just a new idea, so I'm open for feedback and other ideas

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Sorry, I am not sure what you mean. Could you give an example with the new syntax?

Copy link
Member

Choose a reason for hiding this comment

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

The new syntax is using the comparisonDate in the object.

So we would have;

describe('isBefore'
   it('should validate dates against the current date'
     test({
       validator: 'isBefore',
       valid: ,
       invalid:
     })
     test({
       validator: 'isBefore',
       args: [],
       valid: ,
       invalid:
      })
   it('should validate dates against an end date'
     test({
       validator: 'isBefore',
       args: [{ comparisonDate: ],
       valid: ,
       invalid:
      })
   describe('(legacy syntax)'
     it('should validate dates against an end date'
       test({
         validator: 'isBefore',
         args: ['08/04/2011'],
         valid: ,
         invalid:
        })

Is that clear?

Copy link
Contributor Author

@pixelbucket-dev pixelbucket-dev Feb 6, 2023

Choose a reason for hiding this comment

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

I've just pushed a proposed refactor to the tests. Let me know what you think :). Here you can also see how important it is to write test cases for all possible scenarios and permutations, ideally upfront, before implementation. That would have made it easier to refactor the implementation to the new syntax. The existing tests probably still lack possible values or permutations. We should probably also test against other nullish values. E.g. should we guard against null? Because the code will break if options === null.

test({
validator: 'isBefore',
args: [{ comparisonDate: '08/04/2011' }],
valid: ['2010-07-02', '2010-08-04', new Date(0).toString()],
invalid: ['08/04/2011', new Date(2011, 9, 10).toString()],
});
test({
validator: 'isBefore',
args: [{ comparisonDate: new Date(2011, 7, 4).toString() }],
valid: ['2010-07-02', '2010-08-04', new Date(0).toString()],
invalid: ['08/04/2011', new Date(2011, 9, 10).toString()],
});
test({
validator: 'isBefore',
valid: [
'2000-08-04',
new Date(0).toString(),
new Date(Date.now() - 86400000).toString(),
],
invalid: ['2100-07-02', new Date(2217, 10, 10).toString()],
});
test({
validator: 'isBefore',
args: [{ comparisonDate: '2011-08-03' }],
valid: ['1999-12-31'],
invalid: ['invalid date'],
});
test({
validator: 'isBefore',
args: [{ comparisonDate: 'invalid date' }],
invalid: ['invalid date', '1999-12-31'],
});
});

describe('(legacy syntax)', () => {
it('should validate dates against an end date', () => {
test({
validator: 'isBefore',
args: ['08/04/2011'],
valid: ['2010-07-02', '2010-08-04', new Date(0).toString()],
invalid: ['08/04/2011', new Date(2011, 9, 10).toString()],
});
test({
validator: 'isBefore',
args: [new Date(2011, 7, 4).toString()],
valid: ['2010-07-02', '2010-08-04', new Date(0).toString()],
invalid: ['08/04/2011', new Date(2011, 9, 10).toString()],
});
test({
validator: 'isBefore',
valid: [
'2000-08-04',
new Date(0).toString(),
new Date(Date.now() - 86400000).toString(),
],
invalid: ['2100-07-02', new Date(2217, 10, 10).toString()],
});
test({
validator: 'isBefore',
args: ['2011-08-03'],
valid: ['1999-12-31'],
invalid: ['invalid date'],
});
test({
validator: 'isBefore',
args: ['invalid date'],
invalid: ['invalid date', '1999-12-31'],
});
});
});
});