add validator and test for date format #75
Conversation
There was a problem hiding this comment.
This testing strategy isn't going to work in the long run. You used the JSON Schema Test Suite as a reference to manually create tests. It needs to programmatically generate the tests from the test suite. The reason is that when the test suite changes, this tests will get out of sync.
See, https://github.com/hyperjump-io/json-schema/blob/main/draft-2020-12/json-schema-test-suite.spec.ts as an example. You should be able to copy that test runner and make minor changes to point to the right tests and enable format validation.
This will force you to test from the public API. Instead of testing the date function, you would be testing a schema with a format keyword. This is a good thing. Testing from the public API makes your tests more resilient to refactoring by decoupling the test from the internal details of how it was implemented.
You did a fantastic job at writing great test descriptions. Most people do a terrible job at that. I feel bad that I'm asking you not to use them.
The draft-2020-12 folder isn't the right place for the format code because it will end up being used in all of the dialects, not just 2020-12. I think putting it in lib makes sense. Also, I don't think it needs to be in an "optional" folder.
Right now, you have the implementation in a file called date.js, but it looks like the intention is for this to encompass all format validators. You aren't exporting the date validation function you export a structure containing a collection of validators. Let's give that file a name that matches what it does.
| /** | ||
| * Function to validate the date-time format using spacetime. | ||
| * @param {string} dateTimeString - The date-time string to validate. | ||
| * @returns {boolean} - True if the date-time string is valid, false otherwise. | ||
| */ |
There was a problem hiding this comment.
This comment isn't helpful. It doesn't say anything that isn't obvious from the name of the function and its parameters. Comments are great when they're communicating something that can't be communicated through the code, but this isn't one of those cases.
| return true; | ||
| } | ||
|
|
||
| const regex = /^\d{4}-(0[1-9]|1[0-2])-(0[1-9]|[12][0-9]|3[01])$/; |
There was a problem hiding this comment.
I generally avoid declaring regular expressions within a function. Declaring it in the function could result in the regular expression needing to be compiled every time the function is called.
In practice, it's not likely to make a noticeable difference because the regex is fairly simple and it might end up getting cached and optimized by the JavaScript engine anyway, but I still think it's a good practice to move the regex out of the function.
related to Issue #72