validator: date normalizer #624
Conversation
carlbrugger
force-pushed
the
feat/DateNormalizer
branch
from
d81daf7 to
fcd6caf
Compare
![flatfile-nullify[bot]](https://avatars.githubusercontent.com/u/31830338?s=60&v=4){kind=small}
WalkthroughThe pull request involves significant modifications to the Changes
Possibly related PRs
🪧 TipsChatThere are 3 ways to chat with CodeRabbit:
Note: Be mindful of the bot's finite context window. It's strongly recommended to break down tasks such as reading entire modules into smaller chunks. For a focused discussion, use review comments to chat about specific files and their changes, instead of using the PR comments. CodeRabbit Commands (Invoked using PR comments)
Other keywords and placeholders
CodeRabbit Configuration File (
|
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 9
🧹 Outside diff range and nitpick comments (5)
validators/date/rollup.config.mjs (1)
1-5: Consider adding a brief comment explaining the file's purpose.While the code is clear and concise, adding a brief comment at the beginning of the file explaining its purpose and any specific choices made would improve maintainability. For example:
// Rollup configuration for the date validator // Uses default settings from @flatfile/rollup-configThis comment would provide context for future developers working on this file.
validators/date/README.md (3)
10-22: Adjust heading levels for consistency.The parameters section is informative and well-structured. However, there's a minor formatting issue with the heading levels.
To maintain consistent heading hierarchy, please update the parameter headings from h4 to h3. Apply this change to all parameter headings:
-#### `config.sheetSlug` - `string` - `default: **` - (optional) +### `config.sheetSlug` - `string` - `default: **` - (optional)🧰 Tools
🪛 Markdownlint
12-12: Expected: h3; Actual: h4
Heading levels should only increment by one level at a time(MD001, heading-increment)
36-56: LGTM: Example usage is clear and comprehensive.The example effectively demonstrates how to configure and use the plugin with all available parameters.
Consider adding a brief comment explaining the expected outcome of this configuration, such as:
// This configuration will normalize 'birthdate' and 'registration_date' fields // in the 'contacts' sheet to the format 'MM/DD/YYYY' without including time.This addition would further clarify the expected behavior for users.
1-56: Excellent documentation for the new date validator plugin.The README provides comprehensive and well-structured documentation for the
@flatfile/plugin-validate-dateplugin. It effectively covers the plugin's purpose, configuration parameters, installation process, and usage example.To further enhance the documentation, consider adding a section on "Supported Date Formats" or "Limitations" to provide users with information on:
- The range of date formats the plugin can detect and normalize.
- Any limitations or edge cases users should be aware of when using the plugin.
This additional information would help users understand the full capabilities and potential constraints of the date normalizer.
🧰 Tools
🪛 Markdownlint
12-12: Expected: h3; Actual: h4
Heading levels should only increment by one level at a time(MD001, heading-increment)
validators/date/src/index.ts (1)
35-35: Use a logging mechanism instead of 'console.error'Using
console.errorin production code is not recommended. Consider replacing it with a dedicated logging library or integrating with your application's error reporting system.
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
⛔ Files ignored due to path filters (2)
package-lock.jsonis excluded by!**/package-lock.json,!**/*.jsonvalidators/date/package.jsonis excluded by!**/*.json
📒 Files selected for processing (5)
- flatfilers/sandbox/src/index.ts (1 hunks)
- validators/date/README.md (1 hunks)
- validators/date/rollup.config.mjs (1 hunks)
- validators/date/src/index.e2e.spec.ts (1 hunks)
- validators/date/src/index.ts (1 hunks)
🧰 Additional context used
🪛 Markdownlint
validators/date/README.md
12-12: Expected: h3; Actual: h4
Heading levels should only increment by one level at a time(MD001, heading-increment)
🔇 Additional comments (5)
validators/date/rollup.config.mjs (3)
1-1: LGTM: Import statement is correct and follows best practices.The import of
buildConfigfrom '@flatfile/rollup-config' is appropriate and likely ensures consistency with other parts of the project.
5-5: LGTM: Default export is correct.The configuration is properly exported as the default export, following common JavaScript module patterns.
3-3: Verify if default configuration is sufficient for date validator.The use of
buildConfig({})is correct and consistent with the import. However, consider if any specific configurations are needed for the date validator. If not, explicitly mentioning that default configuration is sufficient would improve clarity.To ensure we're not missing any required configurations, let's check for any existing Rollup configurations in the project:
✅ Verification successful
Default configuration is sufficient for the date validator.
The use of
buildConfig({})aligns with other Rollup configurations in the project where default settings are appropriate. No specific configurations are required for the date validator.🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash # Description: Check for existing Rollup configurations in the project # Search for rollup config files echo "Searching for Rollup config files:" fd -e js -e mjs -e ts -e cjs rollup.config # If any config files are found, display their content fd -e js -e mjs -e ts -e cjs rollup.config -x echo "Content of {}:" \; -x cat {}Length of output: 4191
validators/date/README.md (2)
1-8: LGTM: Info card section is clear and informative.The info card provides a concise description of the plugin's functionality and correctly specifies the event type it listens to.
24-34: LGTM: Usage instructions are clear and correct.The installation command and import statement are provided correctly, making it easy for users to integrate the plugin into their projects.
There was a problem hiding this comment.
Actionable comments posted: 2
🧹 Outside diff range and nitpick comments (9)
validate/date/src/validate.date.utils.ts (1)
10-28: Overall implementation looks good, with some suggestions for improvement.The function effectively uses chrono-node for parsing and date-fns for formatting, with appropriate error handling. However, there are a couple of areas that could be improved:
- The handling of the
includeTimeoption could be more robust.- There's no validation of the
configobject or its properties.Consider the following improvements:
- Use date-fns functions for handling the
includeTimeoption:import { format, startOfDay } from 'date-fns' // Inside the function const dateToFormat = config.includeTime ? parsedDate : startOfDay(parsedDate) const formattedDate = format(dateToFormat, config.outputFormat, { locale: enUS, })
- Add input validation for the
configobject:if (!config || typeof config.outputFormat !== 'string' || typeof config.includeTime !== 'boolean') { throw new Error('Invalid configuration object') }These changes will make the function more robust and less prone to errors from invalid input.
validate/date/README.md (3)
10-22: Adjust heading levels for consistency.The parameters section is well-documented, but there's an inconsistency in the heading levels. The parameter headings should be h3 instead of h4 for proper document structure.
Apply this change to all parameter headings:
-#### `config.sheetSlug` - `string` - `default: **` - (optional) +### `config.sheetSlug` - `string` - `default: **` - (optional)🧰 Tools
🪛 Markdownlint
12-12: Expected: h3; Actual: h4
Heading levels should only increment by one level at a time(MD001, heading-increment)
36-56: LGTM: Example usage is comprehensive and well-explained.The example usage section provides a clear and practical demonstration of how to use the plugin. It correctly imports dependencies and shows how to configure the plugin with the parameters described earlier.
Consider adding a brief comment explaining the purpose of each configuration option in the example to further enhance understanding:
validateDate({ sheetSlug: 'contacts', + // Specify which fields contain date values to be normalized dateFields: ['birthdate', 'registration_date'], + // Set the desired output format for normalized dates outputFormat: 'MM/DD/YYYY', + // Exclude time information from the normalized output includeTime: false })
1-56: Overall, excellent README with room for a minor addition.The README provides a comprehensive and well-structured overview of the
@flatfile/plugin-validate-dateplugin. It effectively covers the plugin's functionality, configuration parameters, installation, import, and usage. The content is informative and aligns well with the PR objectives.Consider adding a brief section about compatibility or requirements, such as supported Node.js versions or any other relevant dependencies. This information could be valuable for users integrating the plugin into their projects.
🧰 Tools
🪛 Markdownlint
12-12: Expected: h3; Actual: h4
Heading levels should only increment by one level at a time(MD001, heading-increment)
validate/date/src/validate.date.plugin.ts (1)
5-11: Consider making 'outputFormat' and 'includeTime' optional with default valuesTo enhance usability, you might consider making
outputFormatandincludeTimeoptional inDateFormatNormalizerConfigand provide default values if they are not specified.Apply this change to make the properties optional:
export interface DateFormatNormalizerConfig { sheetSlug?: string dateFields: string[] - outputFormat: string - includeTime: boolean + outputFormat?: string + includeTime?: boolean locale?: string }Additionally, you may set default values within the
validateDatefunction if these properties are undefined.validate/date/src/validate.date.plugin.spec.ts (4)
18-19: Unused console spiesThe
logSpyandlogErrorSpyvariables are initialized but not utilized within the tests. If they're not needed, consider removing them to keep the code clean.Apply this diff to remove the unused spies:
- const logSpy = jest.spyOn(global.console, 'log') - const logErrorSpy = jest.spyOn(global.console, 'error')
52-73: Consider adding edge case tests for date normalizationWhile the current tests cover various date formats, consider including edge cases such as leap years (e.g., '2020-02-29'), invalid dates (e.g., '2023-02-30'), and dates with time zones to ensure the
validateDatefunction robustly handles all possible inputs.Would you like assistance in generating additional test cases for these scenarios?
96-115: Include tests for different time zonesIn addition to testing the
includeTimeoption, consider adding test cases that include different time zones (e.g., '2023-01-15T14:30:00Z' or '2023-01-15 14:30:00 +05:00') to verify that the time zone information is correctly parsed or ignored based on the configuration.I can help create these test cases if needed.
134-154: Verify consistent handling of multiple date fieldsThe test effectively verifies that multiple date fields are normalized correctly. Consider adding tests where the date fields have different formats to ensure each field is independently normalized as expected.
Let me know if you'd like assistance in extending this test case.
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
⛔ Files ignored due to path filters (3)
package-lock.jsonis excluded by!**/package-lock.json,!**/*.jsonpackage.jsonis excluded by!**/*.jsonvalidate/date/package.jsonis excluded by!**/*.json
📒 Files selected for processing (7)
- flatfilers/sandbox/src/index.ts (1 hunks)
- validate/date/README.md (1 hunks)
- validate/date/rollup.config.mjs (1 hunks)
- validate/date/src/index.ts (1 hunks)
- validate/date/src/validate.date.plugin.spec.ts (1 hunks)
- validate/date/src/validate.date.plugin.ts (1 hunks)
- validate/date/src/validate.date.utils.ts (1 hunks)
✅ Files skipped from review due to trivial changes (2)
- validate/date/rollup.config.mjs
- validate/date/src/index.ts
🧰 Additional context used
🪛 Markdownlint
validate/date/README.md
12-12: Expected: h3; Actual: h4
Heading levels should only increment by one level at a time(MD001, heading-increment)
🔇 Additional comments (13)
validate/date/src/validate.date.utils.ts (1)
6-9: Function signature looks good.The function name, parameters, and return type are clear, descriptive, and accurately reflect the function's purpose and behavior.
validate/date/README.md (2)
1-8: LGTM: Infocard section is informative and well-structured.The infocard section provides a clear and concise description of the plugin's functionality and correctly specifies the event type it uses.
24-34: LGTM: Usage section is clear and well-formatted.The usage section provides clear installation instructions and a correct import example. The use of language-specific code blocks enhances readability.
flatfilers/sandbox/src/index.ts (4)
1-6: LGTM: Imports and function signature are correct.The import statements and the function signature are appropriate for configuring a Flatfile listener with the required plugins.
7-12: Acknowledge existing issue: Mismatch betweenoutputFormatandincludeTimeAs previously noted, there's a mismatch between the
outputFormatandincludeTimesettings. TheoutputFormatdoesn't include time components, butincludeTimeis set totrue.To resolve this, either update the
outputFormatto include time or setincludeTimetofalse:Option 1:
- outputFormat: 'MM/dd/yyyy', + outputFormat: 'MM/dd/yyyy HH:mm:ss', includeTime: true,Option 2:
outputFormat: 'MM/dd/yyyy', - includeTime: true, + includeTime: false,Choose the option that best fits your requirements for date handling in the 'dob' and 'hire_date' fields.
15-64: Acknowledge existing issue: Inconsistent field types for date fieldsAs previously noted, the 'dob' and 'hire_date' fields are currently defined with
type: 'string', which is inconsistent with their usage as date fields in thevalidateDateplugin.To resolve this, update the field types to 'date':
{ key: 'dob', - type: 'string', + type: 'date', label: 'Date of Birth', }, { key: 'hire_date', - type: 'string', + type: 'date', label: 'Hire Date', },This change will ensure consistency between the field definitions and their usage in the
validateDateplugin.
1-64: Summary: Changes align with PR objectives and introduce new validator structureThe changes in this file successfully introduce a new validator structure as outlined in the PR objectives. The file now uses the
validateDateplugin for date normalization and theconfigureSpaceplugin for defining the workbook and sheet structure. This aligns well with the organizational focus of the PR.Key points:
- The new structure provides a clear separation of concerns between date validation and space configuration.
- The 'Contacts' sheet schema is well-defined, allowing for future expansion with
allowAdditionalFields: true.- Date normalization is specifically targeted at 'dob' and 'hire_date' fields.
There are minor inconsistencies in date handling configurations that have been addressed in previous comments. Once these are resolved, the file will provide a solid foundation for the new validator structure.
validate/date/src/validate.date.plugin.ts (1)
1-3: Import statements are correctly definedThe necessary modules and functions are correctly imported.
validate/date/src/validate.date.plugin.spec.ts (5)
1-10: Well-structured imports and setupThe initial setup with imports and utility functions is clear and organized, facilitating maintainable and readable tests.
24-34: Efficient test environment setupThe
beforeAllhook effectively sets up the testing space and workbook with the necessary fields, ensuring a proper environment for the tests.
40-49: Robust cleanup in afterEach hookThe
afterEachhook appropriately resets the listener and cleans up records, maintaining test isolation and preventing side effects between tests.
75-94: Good handling of invalid date formatsThe test correctly verifies that invalid date formats generate appropriate error messages, ensuring that users receive clear feedback on input errors.
116-133: Test coverage for locale-specific date formatsThe test adequately checks handling of different locales. Ensure that the
validateDatefunction supports all required locales and consider adding tests for additional locales if necessary.
carlbrugger
force-pushed
the
feat/DateNormalizer
branch
from
7da3452 to
77d39ff
Compare
There was a problem hiding this comment.
Actionable comments posted: 1
🧹 Outside diff range and nitpick comments (7)
validate/date/README.md (3)
10-22: Adjust heading levels for better document structure.The parameters section is well-documented, providing clear descriptions for each configuration option. However, the heading levels for individual parameters should be adjusted for better document structure.
Apply this change to fix the heading levels:
## Parameters -#### `config.sheetSlug` - `string` - `default: **` - (optional) +### `config.sheetSlug` - `string` - `default: **` - (optional) The `sheetSlug` parameter is the slug of the sheet to apply the normalizer to. If not provided, it will apply to all sheets. -#### `config.dateFields` - `string[]` +### `config.dateFields` - `string[]` The `dateFields` parameter is an array of field names that contain date values to be normalized. -#### `config.outputFormat` - `string` +### `config.outputFormat` - `string` The `outputFormat` parameter specifies the desired output format for the normalized dates. -#### `config.includeTime` - `boolean` +### `config.includeTime` - `boolean` The `includeTime` parameter determines whether to include time in the normalized output.🧰 Tools
🪛 Markdownlint
12-12: Expected: h3; Actual: h4
Heading levels should only increment by one level at a time(MD001, heading-increment)
36-56: LGTM: Example usage is comprehensive and well-explained.The example usage section provides a clear and practical demonstration of how to configure and use the plugin. The code is well-formatted and easy to understand.
Consider adding a brief comment explaining the purpose of each configuration option in the example. This would further enhance the understanding for users new to the plugin. For example:
validateDate({ sheetSlug: 'contacts', // Apply to the 'contacts' sheet only dateFields: ['birthdate', 'registration_date'], // Fields to normalize outputFormat: 'MM/DD/YYYY', // Desired output format includeTime: false // Exclude time from the output })
1-56: Excellent README with comprehensive documentation.The README provides a thorough and well-structured guide for the
@flatfile/plugin-validate-dateplugin. It covers all essential aspects including description, configuration options, installation instructions, and usage examples.Consider adding a "Features" or "Capabilities" section near the beginning of the document to highlight key functionalities of the plugin, such as:
- Support for multiple date formats
- Locale-aware date parsing
- Customizable output format
- Multi-field support within a single configuration
This addition would give users a quick overview of the plugin's strengths and use cases.
🧰 Tools
🪛 Markdownlint
12-12: Expected: h3; Actual: h4
Heading levels should only increment by one level at a time(MD001, heading-increment)
validate/date/src/validate.date.plugin.spec.ts (4)
14-43: LGTM: Comprehensive test setup and teardown.The test suite setup is well-structured with appropriate beforeAll, afterAll, and afterEach hooks. This ensures a clean testing environment for each test case.
Consider adding error handling in the afterAll and afterEach hooks to ensure cleanup operations complete successfully even if an error occurs:
afterAll(async () => { try { await deleteSpace(spaceId) } catch (error) { console.error('Error during space cleanup:', error) } }) afterEach(async () => { try { listener.reset() const records = await getRecords(sheetId) if (records.length > 0) { const ids = records.map((record) => record.id) await api.records.delete(sheetId, { ids }) } } catch (error) { console.error('Error during test cleanup:', error) } })
45-67: LGTM: Comprehensive test for date normalization.This test case effectively checks the normalization of multiple date formats to a specified output format. It's a good coverage of the
validateDatefunction's core functionality.Consider adding a test case for a date with a time component to ensure it's correctly handled when
includeTimeis set to false. For example:await createRecords(sheetId, [ // ... existing records ... { date: '2023-01-15 14:30:00', name: 'Tom' }, ]) // ... existing assertions ... expect(records[3].values['date'].value).toBe('01/15/2023')This will ensure that the time component is correctly stripped when not required.
69-88: LGTM: Good error handling test.This test case effectively checks that the plugin handles invalid date formats and generates an error message.
To make the test more robust, consider asserting the specific error message content. This ensures that the error message is not only present but also contains the expected information. For example:
expect(records[0].values['date'].messages[0].message).toBe('Unable to parse date string: invalid-date')This way, you're ensuring both the presence of an error message and its specific content, which can be helpful for debugging and maintaining consistent error messages.
90-149: LGTM: Comprehensive coverage of various scenarios.The remaining test cases effectively cover important scenarios such as including time, handling different locales, and normalizing multiple date fields. This ensures the plugin works correctly under various configurations and with different types of input.
Consider adding a test case for handling daylight saving time (DST) transitions. This could be particularly important if the plugin is used with time zones that observe DST. For example:
it('handles daylight saving time transitions', async () => { listener.use( validateDate({ sheetSlug: 'test', dateFields: ['date'], outputFormat: 'MM/dd/yyyy HH:mm:ss', includeTime: true, timeZone: 'America/New_York', // or any timezone that observes DST }) ) await createRecords(sheetId, [ { date: '2023-03-12 01:59:59', name: 'DST Start' }, { date: '2023-03-12 03:00:00', name: 'DST Started' }, { date: '2023-11-05 01:59:59', name: 'DST End' }, { date: '2023-11-05 01:00:00', name: 'DST Ended' }, ]) await listener.waitFor('commit:created') const records = await getRecords(sheetId) expect(records[0].values['date'].value).toBe('03/12/2023 01:59:59') expect(records[1].values['date'].value).toBe('03/12/2023 03:00:00') expect(records[2].values['date'].value).toBe('11/05/2023 01:59:59') expect(records[3].values['date'].value).toBe('11/05/2023 01:00:00') })This test would ensure that the plugin correctly handles dates around DST transitions, which can be a common source of bugs in date/time handling.
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
⛔ Files ignored due to path filters (2)
package-lock.jsonis excluded by!**/package-lock.json,!**/*.jsonvalidate/date/package.jsonis excluded by!**/*.json
📒 Files selected for processing (7)
- flatfilers/sandbox/src/index.ts (2 hunks)
- validate/date/README.md (1 hunks)
- validate/date/rollup.config.mjs (1 hunks)
- validate/date/src/index.ts (1 hunks)
- validate/date/src/validate.date.plugin.spec.ts (1 hunks)
- validate/date/src/validate.date.plugin.ts (1 hunks)
- validate/date/src/validate.date.utils.ts (1 hunks)
🚧 Files skipped from review as they are similar to previous changes (4)
- validate/date/rollup.config.mjs
- validate/date/src/index.ts
- validate/date/src/validate.date.plugin.ts
- validate/date/src/validate.date.utils.ts
🧰 Additional context used
🪛 Markdownlint
validate/date/README.md
12-12: Expected: h3; Actual: h4
Heading levels should only increment by one level at a time(MD001, heading-increment)
🔇 Additional comments (6)
flatfilers/sandbox/src/index.ts (2)
3-3: LGTM: Import statement for validateDate is correct.The import statement for
validateDatefrom '@flatfile/plugin-validate-date' is correctly added and aligns with its usage in the file.
50-59: 🛠️ Refactor suggestionConsider changing field types for 'dob' and 'hire_date' to 'date'.
The 'dob' and 'hire_date' fields are currently defined with
type: 'string'. Since these fields represent dates and you're using thevalidateDateplugin, it would be more appropriate to define their types as'date'.Apply this diff to update the field types:
{ key: 'dob', - type: 'string', + type: 'date', label: 'Date of Birth', }, { key: 'hire_date', - type: 'string', + type: 'date', label: 'Hire Date', },This change will ensure proper handling and validation of date values in the Flatfile system.
Likely invalid or redundant comment.
validate/date/README.md (2)
1-8: LGTM: Infocard section is informative and well-structured.The infocard section provides a clear and concise description of the plugin's functionality and specifies the event type it uses. This information is crucial for users to understand the plugin's purpose and integration point.
24-34: LGTM: Usage section is clear and well-formatted.The usage section provides clear installation instructions and an import example. The use of code blocks for both bash and JavaScript makes it easy for users to understand how to install and import the plugin.
validate/date/src/validate.date.plugin.spec.ts (2)
1-13: LGTM: Imports and initial setup are appropriate.The necessary imports from '@flatfile/api', '@flatfile/utils-testing', and the local
validateDatefunction are correctly included. The creation of theFlatfileClientinstance is also appropriate for API interactions in the tests.
1-149: Overall, excellent test coverage for the validateDate plugin.This test suite demonstrates a comprehensive approach to testing the validateDate plugin. It covers a wide range of scenarios including various date formats, error handling, time inclusion, different locales, and multiple field normalization. The structure of the tests, with proper setup and teardown procedures, follows best practices for maintainable and reliable tests.
The thoroughness of these tests is crucial for ensuring the reliability and robustness of the validateDate plugin. As date handling can be complex due to various formats, locales, and edge cases, this level of testing is essential for catching potential issues and maintaining the plugin's functionality across different use cases.
While the test suite is already strong, the suggested improvements (such as additional error handling in setup/teardown, more precise error message checking, and considering DST transitions) could further enhance its effectiveness. These additions would help catch even more edge cases and ensure consistent behavior across all scenarios.
Great job on creating a solid foundation for testing this critical functionality!
Please explain how to summarize this PR for the Changelog:
Tell code reviewer how and what to test: