Conversation
WalkthroughThe changes introduce a new Flatfile Number Validation Plugin that provides extensive validation features for numeric data during data import. Key functionalities include checks for minimum and maximum values, integer-only constraints, precision and scale, currency formatting, step validation, and special number types. New configuration files for Jest and Rollup are added, along with comprehensive end-to-end and unit tests for the validation function. The main validation logic is encapsulated in the Changes
Sequence Diagram(s)sequenceDiagram
participant User
participant FlatfileListener
participant ValidateNumber
User->>FlatfileListener: Create record
FlatfileListener->>ValidateNumber: Trigger 'commit:created' event
ValidateNumber->>ValidateNumber: Validate number field
ValidateNumber->>ValidateNumber: Check min/max, integer, precision, scale, etc.
ValidateNumber-->>FlatfileListener: Log success/error
Possibly related PRs
Suggested reviewers
🪧 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: 14
🧹 Outside diff range and nitpick comments (3)
validate/number/rollup.config.mjs (1)
1-5: Consider adding comments for better documentation.While the code is straightforward, adding a brief comment explaining the purpose of this configuration file and any specific settings (if any) would improve maintainability.
validate/number/src/ValidateNumber.e2e.spec.ts (1)
174-189: Consider verifying validation messages for comprehensive testingCurrently, the tests check if the records are valid or invalid. To ensure that the validation rules are functioning as expected, consider asserting that the correct error messages are assigned to invalid records.
Example of how to extend the assertions:
expect(recordByAmount[-10].messages.amount).toContain('Value must be greater than or equal to 0')validate/number/src/index.ts (1)
121-140: EnsurenumberValueis an integer when checking 'prime', 'even', or 'odd'When validating special types like 'prime', 'even', or 'odd', it's important that
numberValueis an integer. Non-integer values may lead to incorrect validations.Consider adding a check to confirm that
numberValueis an integer before performing these validations:+ if (!Number.isInteger(numberValue)) { + throw new Error(`The field '${field}' must be an integer for '${config.specialTypes.join(", ")}' validation`) + } if (config.specialTypes) { if ( config.specialTypes.includes('prime') && !isPrime(numberValue) ) {Alternatively, ensure that
integerOnlyis set totruewhen using these special types.
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
⛔ Files ignored due to path filters (2)
validate/number/metadata.jsonis excluded by!**/*.jsonvalidate/number/package.jsonis excluded by!**/*.json
📒 Files selected for processing (4)
- validate/number/README.MD (1 hunks)
- validate/number/rollup.config.mjs (1 hunks)
- validate/number/src/ValidateNumber.e2e.spec.ts (1 hunks)
- validate/number/src/index.ts (1 hunks)
🧰 Additional context used
🪛 LanguageTool
validate/number/README.MD
[uncategorized] ~55-~55: Loose punctuation mark.
Context: ...ect with the following options: -min: Minimum allowed value -max: Maximum ...(UNLIKELY_OPENING_PUNCTUATION)
🪛 Biome
validate/number/src/index.ts
[error] 38-38: isNaN is unsafe. It attempts a type coercion. Use Number.isNaN instead.
See the MDN documentation for more details.
Unsafe fix: Use Number.isNaN instead.(lint/suspicious/noGlobalIsNan)
🔇 Additional comments (3)
validate/number/rollup.config.mjs (2)
1-5: LGTM! Clean and concise Rollup configuration.The implementation is correct and follows best practices for setting up a Rollup configuration using the
@flatfile/rollup-configpackage.
3-3: Verify if default configuration is sufficient for the number validation plugin.The
buildConfigfunction is called with an empty object, which suggests using default settings. Please confirm that the default configuration meets all requirements for building the number validation plugin.✅ Verification successful
Default configuration is sufficient for the number validation plugin.
No additional Rollup configurations are required based on the current codebase analysis.🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash # Description: Check if there are any specific Rollup configuration needs for the number validation plugin # Test 1: Look for any Rollup-related comments or TODOs in the plugin code echo "Searching for Rollup-related comments or TODOs in the plugin code:" rg --type js --type typescript "(?i)(rollup|build|bundle).*todo" ./validate/number # Test 2: Check if there are any non-standard file types or locations that might need special Rollup handling echo "Checking for non-standard file types or locations:" fd -t f . ./validate/number | grep -vE '\.(js|ts|mjs|cjs)$' # Test 3: Look for any import statements that might require special Rollup plugins echo "Checking for import statements that might require special Rollup plugins:" rg --type js --type typescript "import.*from" ./validate/number | grep -vE "from '(@flatfile/|\./)'"Length of output: 827
Script:
#!/bin/bash # Description: Check if there are any specific Rollup configuration needs for the number validation plugin # Test 1: Look for any Rollup-related comments or TODOs in the plugin code echo "Searching for Rollup-related comments or TODOs in the plugin code:" rg --type js --type ts "(?i)(rollup|build|bundle).*todo" ./validate/number # Test 2: Check if there are any non-standard file types or locations that might need special Rollup handling echo "Checking for non-standard file types or locations:" fd -t f . ./validate/number | grep -vE '\.(js|ts|mjs|cjs)$' # Test 3: Look for any import statements that might require special Rollup plugins echo "Checking for import statements that might require special Rollup plugins:" rg --type js --type ts "import.*from" ./validate/number | grep -vE "from '(@flatfile/|\./)'"Length of output: 1205
validate/number/README.MD (1)
29-29: Ensure the import statement uses the correct package name.Verify that the import statement uses the correct package name matching the installation command. If the package name is corrected to
@flatfile/plugin-validate-number, ensure the import reflects this change.Apply this diff if needed:
- import { validateNumber } from '@flatfile/plugin-validate-number'; + import { validateNumber } from '@flatfile/plugin-validate-number';
There was a problem hiding this comment.
Actionable comments posted: 6
🧹 Outside diff range and nitpick comments (1)
validate/number/src/index.ts (1)
3-19: Consider adding JSDoc comments to theNumberValidationConfiginterfaceThe
NumberValidationConfiginterface is well-structured, but adding JSDoc comments would improve its documentation and provide better context for each property. This is especially useful for properties with non-obvious purposes or constraints.Here's an example of how you could add JSDoc comments:
/** * Configuration for number validation */ interface NumberValidationConfig { /** Fields to validate */ fields: string[] /** Minimum allowed value */ min?: number /** Maximum allowed value */ max?: number /** Whether min and max are inclusive */ inclusive?: boolean /** Allow only integer values */ integerOnly?: boolean /** Total number of digits allowed */ precision?: number /** Number of decimal places allowed */ scale?: number /** Validate as currency (implies two decimal places) */ currency?: boolean /** Value must be a multiple of this step */ step?: number /** Character used as thousands separator */ thousandsSeparator?: string /** Character used as decimal point */ decimalPoint?: string /** Special number types to validate (e.g., 'prime', 'even', 'odd') */ specialTypes?: string[] /** Round the number before validation */ round?: boolean /** Truncate the number before validation */ truncate?: boolean /** Specific sheet to apply validation to */ sheetSlug?: string }
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
📒 Files selected for processing (2)
- validate/number/src/ValidateNumber.e2e.spec.ts (1 hunks)
- validate/number/src/index.ts (1 hunks)
🚧 Files skipped from review as they are similar to previous changes (1)
- validate/number/src/ValidateNumber.e2e.spec.ts
🧰 Additional context used
🪛 Biome
validate/number/src/index.ts
[error] 34-34: isNaN is unsafe. It attempts a type coercion. Use Number.isNaN instead.
See the MDN documentation for more details.
Unsafe fix: Use Number.isNaN instead.(lint/suspicious/noGlobalIsNan)
🔇 Additional comments (1)
validate/number/src/index.ts (1)
1-170: Overall assessment: Solid foundation with room for improvementThe
validateNumberfunction and its supporting code provide a comprehensive solution for number validation. The implementation covers a wide range of validation scenarios and is well-structured. However, there are several areas where improvements can be made:
- Enhanced error handling and type safety
- Optimized parsing and validation logic
- More robust handling of edge cases
- Improved documentation
By implementing the suggested changes, the code will become more robust, efficient, and maintainable. The resulting validator will be better equipped to handle various number formats and validation requirements, providing a valuable tool for data validation in the Flatfile ecosystem.
🧰 Tools
🪛 Biome
[error] 34-34: isNaN is unsafe. It attempts a type coercion. Use Number.isNaN instead.
See the MDN documentation for more details.
Unsafe fix: Use Number.isNaN instead.(lint/suspicious/noGlobalIsNan)
There was a problem hiding this comment.
Actionable comments posted: 1
🧹 Outside diff range and nitpick comments (2)
validate/number/README.MD (2)
31-55: LGTM: Clear and comprehensive usage example.The example effectively demonstrates how to import and use the plugin with various configuration options. It provides a good starting point for users.
Consider adding brief inline comments explaining each configuration option in the example. This would help users understand the purpose of each setting without needing to refer to the configuration section.
1-81: Overall, the README is well-structured and informative.The document provides clear installation instructions, a comprehensive usage example, detailed configuration options, and a concise behavior description. Most issues from previous reviews have been addressed, improving the overall quality of the documentation.
To further enhance the README:
- Consider adding the
fieldNameoption to the configuration list as suggested earlier.- You might want to add a brief "Getting Started" section after the installation instructions to guide users through the basic setup process.
Would you like assistance in drafting these additions or making any other improvements to the README?
🧰 Tools
🪛 LanguageTool
[uncategorized] ~61-~61: Loose punctuation mark.
Context: ...ect with the following options: -min: Minimum allowed value -max: Maximum ...(UNLIKELY_OPENING_PUNCTUATION)
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
📒 Files selected for processing (1)
- validate/number/README.MD (1 hunks)
🧰 Additional context used
🪛 LanguageTool
validate/number/README.MD
[uncategorized] ~61-~61: Loose punctuation mark.
Context: ...ect with the following options: -min: Minimum allowed value -max: Maximum ...(UNLIKELY_OPENING_PUNCTUATION)
🔇 Additional comments (3)
validate/number/README.MD (3)
1-22: LGTM: Clear introduction and comprehensive features list.The introduction provides a concise overview of the plugin's purpose, and the features list is comprehensive, accurately reflecting the plugin's capabilities.
23-29: LGTM: Installation instructions are correct and consistent.The installation instructions are clear and the package name in the npm command now correctly matches the import statement (
@flatfile/plugin-validate-number). This resolves the inconsistency noted in a previous review.
75-81: LGTM: Clear description of plugin behavior.The behavior section provides a concise and informative explanation of how the plugin operates, including details about event listening, validation process, and error handling. The mention of the default field name 'numberField' is consistent with the earlier suggestion to add a
fieldNameoption to the configuration list.
bangarang
force-pushed
the
feat/NumberValidator
branch
from
3030f44 to
8976234
Compare
There was a problem hiding this comment.
Actionable comments posted: 6
🧹 Outside diff range and nitpick comments (7)
validate/number/src/index.ts (1)
1-19: LGTM! Consider adding JSDoc comments for better documentation.The import statements and interface definition look good. The
NumberValidationConfiginterface is well-structured with appropriate types for each option.To improve documentation, consider adding JSDoc comments to the interface and its properties. This will provide better intellisense support and make the code more self-documenting. For example:
/** * Configuration options for number validation. */ interface NumberValidationConfig { /** Fields to validate */ fields: string[] /** Minimum allowed value */ min?: number // ... (add comments for other properties) }validate/number/src/ValidateNumber.e2e.spec.ts (6)
44-70: LGTM with a suggestion: Min and max validation test is comprehensive.The test case for min and max validation is well-structured and covers the essential scenarios. However, to improve test reliability:
Consider matching records by their
amountvalue instead of relying on array indices. This ensures the test remains accurate even if the order of returned records changes. Apply this diff:- expect(records[0].values.amount?.messages?.[0].message).toContain( - 'must be greater than' - ) - expect(records[1].values.amount.messages).toEqual([]) - expect(records[2].values.amount.messages?.[0].message).toContain( - 'must be less than' - ) + const recordByAmount = Object.fromEntries(records.map(r => [r.values.amount.value, r])) + expect(recordByAmount[-10].values.amount?.messages?.[0].message).toContain( + 'must be greater than' + ) + expect(recordByAmount[50].values.amount.messages).toEqual([]) + expect(recordByAmount[150].values.amount.messages?.[0].message).toContain( + 'must be less than' + )
72-94: LGTM with a suggestion: Integer-only validation test is thorough.The test case for integer-only validation is well-designed and covers essential scenarios, including the handling of integers, decimals, and string inputs. However, to enhance test reliability:
Consider matching records by their
quantityvalue instead of relying on array indices. This ensures the test remains accurate even if the order of returned records changes. Apply this diff:- expect(records[0].values.quantity.messages).toEqual([]) - expect(records[1].values.quantity.messages?.[0].message).toContain( - 'must be an integer' - ) - expect(records[2].values.quantity.value).toEqual(3) + const recordByQuantity = Object.fromEntries(records.map(r => [r.values.quantity.value, r])) + expect(recordByQuantity[5].values.quantity.messages).toEqual([]) + expect(recordByQuantity[3.14].values.quantity.messages?.[0].message).toContain( + 'must be an integer' + ) + expect(recordByQuantity[3].values.quantity.value).toEqual(3)
96-121: LGTM with a suggestion: Precision and scale validation test is comprehensive.The test case for precision and scale validation is well-structured and covers the essential scenarios. However, to improve test reliability:
Consider matching records by their
pricevalue instead of relying on array indices. This ensures the test remains accurate even if the order of returned records changes. Apply this diff:- expect(records[0].values.price.messages).toEqual([]) - expect(records[1].values.price.messages?.[0].message).toContain( - 'must have at most 3 digits before the decimal point' - ) - expect(records[2].values.price.messages?.[0].message).toContain( - 'must have at most 2 digits after the decimal point' - ) + const recordByPrice = Object.fromEntries(records.map(r => [r.values.price.value, r])) + expect(recordByPrice[123.45].values.price.messages).toEqual([]) + expect(recordByPrice[1234.56].values.price.messages?.[0].message).toContain( + 'must have at most 3 digits before the decimal point' + ) + expect(recordByPrice[12.345].values.price.messages?.[0].message).toContain( + 'must have at most 2 digits after the decimal point' + )
123-145: LGTM with a suggestion: Step value validation test is well-designed.The test case for step value validation is comprehensive and covers the essential scenarios. However, to enhance test reliability:
Consider matching records by their
amountvalue instead of relying on array indices. This ensures the test remains accurate even if the order of returned records changes. Apply this diff:- expect(records[0].values.amount.messages).toEqual([]) - expect(records[1].values.amount.messages?.[0].message).toContain( - 'must be a multiple of 0.5' - ) - expect(records[2].values.amount.messages).toEqual([]) + const recordByAmount = Object.fromEntries(records.map(r => [r.values.amount.value, r])) + expect(recordByAmount[1.5].values.amount.messages).toEqual([]) + expect(recordByAmount[2.25].values.amount.messages?.[0].message).toContain( + 'must be a multiple of 0.5' + ) + expect(recordByAmount[3].values.amount.messages).toEqual([])
147-169: LGTM with a suggestion: Currency value handling test is thorough.The test case for handling currency values is well-structured and covers the essential scenarios. However, to improve test reliability:
Consider matching records by their
pricevalue instead of relying on array indices. This ensures the test remains accurate even if the order of returned records changes. Apply this diff:- expect(records[0].values.price.messages).toEqual([]) - expect(records[1].values.price.messages).toEqual([]) - expect(records[2].values.price.messages?.[0].message).toContain( - 'must be a valid currency value' - ) + const recordByPrice = Object.fromEntries(records.map(r => [r.values.price.value, r])) + expect(recordByPrice[10.0].values.price.messages).toEqual([]) + expect(recordByPrice[15.5].values.price.messages).toEqual([]) + expect(recordByPrice[20.555].values.price.messages?.[0].message).toContain( + 'must be a valid currency value' + )
171-193: LGTM with a suggestion: Special number types validation test is comprehensive.The test case for validating special number types (even numbers in this case) is well-designed and covers the essential scenarios. However, to enhance test reliability:
Consider matching records by their
amountvalue instead of relying on array indices. This ensures the test remains accurate even if the order of returned records changes. Apply this diff:- expect(records[0].values.amount.messages).toEqual([]) - expect(records[1].values.amount.messages?.[0].message).toContain( - 'must be an even number' - ) - expect(records[2].values.amount.messages).toEqual([]) + const recordByAmount = Object.fromEntries(records.map(r => [r.values.amount.value, r])) + expect(recordByAmount[2].values.amount.messages).toEqual([]) + expect(recordByAmount[3].values.amount.messages?.[0].message).toContain( + 'must be an even number' + ) + expect(recordByAmount[4].values.amount.messages).toEqual([])
📜 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/number/package.jsonis excluded by!**/*.json
📒 Files selected for processing (4)
- validate/number/README.MD (1 hunks)
- validate/number/rollup.config.mjs (1 hunks)
- validate/number/src/ValidateNumber.e2e.spec.ts (1 hunks)
- validate/number/src/index.ts (1 hunks)
🚧 Files skipped from review as they are similar to previous changes (1)
- validate/number/rollup.config.mjs
🧰 Additional context used
🪛 LanguageTool
validate/number/README.MD
[uncategorized] ~61-~61: Loose punctuation mark.
Context: ...ect with the following options: -min: Minimum allowed value -max: Maximum ...(UNLIKELY_OPENING_PUNCTUATION)
🪛 Biome
validate/number/src/index.ts
[error] 34-34: isNaN is unsafe. It attempts a type coercion. Use Number.isNaN instead.
See the MDN documentation for more details.
Unsafe fix: Use Number.isNaN instead.(lint/suspicious/noGlobalIsNan)
🔇 Additional comments (11)
validate/number/README.MD (4)
1-22: LGTM: Comprehensive introduction and features list.The introduction clearly explains the plugin's purpose, and the features list provides a comprehensive overview of the plugin's capabilities. This section effectively communicates the value proposition of the plugin to potential users.
23-30: LGTM: Clear installation instructions with correct package name.The installation section provides clear instructions using npm. The package name in the installation command (
@flatfile/plugin-validate-number) is correct and consistent with the import statement in the usage example. This addresses a previous review comment about package name inconsistency.
31-55: LGTM: Comprehensive and clear usage example.The example usage section effectively demonstrates how to import and use the plugin with a FlatfileListener. The configuration object in the example showcases a wide range of the plugin's capabilities, aligning well with both the features list and the configuration options described later in the README. This provides users with a clear understanding of how to implement the plugin in their projects.
75-81: LGTM: Clear and informative behavior description.The behavior section effectively explains how the plugin interacts with the Flatfile system. It provides valuable information about:
- The event the plugin listens for ('commit:created')
- The default field name ('numberField')
- Number parsing considerations
- Rounding and truncation options
- Error handling and logging
This information helps users understand the plugin's operation within their data import processes.
validate/number/src/index.ts (1)
46-81: LGTM! Comprehensive validation logic for integer, min, and max checksThe validation logic for integer-only values, minimum, and maximum constraints is well-implemented. The handling of inclusive and exclusive ranges is flexible and the error messages are clear and informative.
The use of
config.inclusiveprovides good flexibility in range definition. Keep up the good work!validate/number/src/ValidateNumber.e2e.spec.ts (6)
1-13: LGTM: Imports and initial setup are appropriate.The import statements and initial setup look good. They include all necessary dependencies for the end-to-end tests, including the FlatfileClient and testing utilities.
14-42: LGTM: Test suite setup and teardown are well-structured.The test suite setup and teardown processes are comprehensive and follow best practices:
- Proper use of Jest's
describe,beforeAll, andafterAllhooks.- Creation of a test space, workbook, and sheet.
- Cleanup after each test to ensure a clean state.
This structure ensures reliable and isolated test execution.
195-210: LGTM: Rounding handling test is well-implemented.The test case for handling rounding is concise and effectively tests the rounding functionality. It covers both rounding down (1.4 to 1) and rounding up (1.6 to 2), which are the essential scenarios for this feature.
212-227: LGTM: Truncation handling test is well-implemented.The test case for handling truncation is concise and effectively tests the truncation functionality. It covers truncation of both 1.4 and 1.6 to 1, which are the essential scenarios for this feature.
229-250: LGTM: Custom separators handling test is well-implemented.The test case for handling custom separators is comprehensive and effectively tests the parsing of numbers with different thousand separators and decimal points. It covers both the European format (1.000,50) and a mixed format (2,500.75), ensuring that the
validateNumberfunction correctly interprets and stores these values.
1-250: Overall, the test suite is well-structured and comprehensive.This end-to-end test suite for the
validateNumberfunction is thorough and well-implemented. It covers all major functionalities including min/max validation, integer-only checks, precision and scale, step values, currency handling, special number types, rounding, truncation, and custom separators. The tests are clear, concise, and follow best practices for Jest and Flatfile testing.The suggested improvements for matching records by their values instead of array indices will further enhance the reliability of the tests, making them more robust against potential changes in record order.
Great job on creating a comprehensive test suite that will help ensure the reliability and correctness of the
validateNumberfunction!
There was a problem hiding this comment.
Actionable comments posted: 10
🧹 Outside diff range and nitpick comments (1)
validate/number/src/validate.number.plugin.ts (1)
145-146: Correct the warning message for odd number validation.Ensure the warning message accurately reflects the validation. Currently, the message is correct, but double-check for consistency and clarity.
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
📒 Files selected for processing (3)
- validate/number/src/index.ts (1 hunks)
- validate/number/src/validate.number.plugin.e2e.spec.ts (1 hunks)
- validate/number/src/validate.number.plugin.ts (1 hunks)
✅ Files skipped from review due to trivial changes (1)
- validate/number/src/index.ts
🧰 Additional context used
🪛 Biome
validate/number/src/validate.number.plugin.ts
[error] 34-34: isNaN is unsafe. It attempts a type coercion. Use Number.isNaN instead.
See the MDN documentation for more details.
Unsafe fix: Use Number.isNaN instead.(lint/suspicious/noGlobalIsNan)
🔇 Additional comments (2)
validate/number/src/validate.number.plugin.ts (1)
38-44: Clarify the precedence betweenroundandtruncateoptions.If both
roundandtruncateare set totrue,truncatewill override the result ofround. Verify if this is the intended behavior. If not, consider making these options mutually exclusive or defining a specific order.Consider updating the code to reflect the intended behavior or adding documentation to specify how these options interact.
validate/number/src/validate.number.plugin.e2e.spec.ts (1)
1-250: Overall ReviewThe end-to-end tests for the
validateNumberfunction are comprehensive and well-structured, effectively covering various validation scenarios. The test cases are well-organized, and the usage of the validation plugin demonstrates correct implementation.
There was a problem hiding this comment.
Actionable comments posted: 1
🧹 Outside diff range and nitpick comments (1)
validate/number/README.MD (1)
31-55: Consider adding comments to the example configurationThe Example Usage section provides a clear demonstration of how to import and use the plugin with various configuration options. To enhance readability and understanding, consider adding brief comments explaining the purpose of each configuration option.
Here's an example of how you could add comments:
validateNumber({ min: 0, // Minimum allowed value max: 1000, // Maximum allowed value inclusive: true, // Include min and max in valid range integerOnly: true, // Allow only integer values precision: 10, // Total number of digits allowed scale: 2, // Number of decimal places allowed currency: true, // Validate as a currency value step: 5, // Allow only multiples of 5 thousandsSeparator: ',', // Use comma as thousands separator decimalPoint: '.', // Use period as decimal point specialTypes: ['even'], // Validate for even numbers round: true // Round to nearest integer })This addition would make the example more informative for users new to the plugin.
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
📒 Files selected for processing (1)
- validate/number/README.MD (1 hunks)
🧰 Additional context used
🪛 LanguageTool
validate/number/README.MD
[uncategorized] ~61-~61: Loose punctuation mark.
Context: ...ect with the following options: -min: Minimum allowed value -max: Maximum ...(UNLIKELY_OPENING_PUNCTUATION)
🔇 Additional comments (3)
validate/number/README.MD (3)
1-9: LGTM: Clear and informative introductionThe header and introduction provide a concise overview of the plugin's purpose and specify the event type it responds to. This information is crucial for users to understand the plugin's functionality at a glance.
11-22: LGTM: Comprehensive feature listThe Features section provides a detailed and well-structured list of the plugin's capabilities. This helps users quickly understand the extent of validation options available to them.
23-29: LGTM: Clear installation instructionsThe Installation section provides clear and correct instructions for installing the plugin using npm. The package name in the installation command now matches the import statement, addressing a previous review comment.
bangarang
force-pushed
the
feat/NumberValidator
branch
from
7d0830d to
9be0704
Compare
There was a problem hiding this comment.
Actionable comments posted: 4
🧹 Outside diff range and nitpick comments (9)
validate/number/src/validate.number.plugin.spec.ts (9)
27-44: LGTM with suggestion: Min and max constraints are well-tested.This test case effectively verifies the behavior of
validateNumberFieldwhen validating against minimum and maximum value constraints. It covers important scenarios including values within range, below minimum, and above maximum.Consider splitting this test into separate test cases for each scenario (within range, below minimum, above maximum) to improve test isolation and readability. For example:
it('should accept values within min and max range', () => { const config: NumberValidationConfig = { min: 0, max: 100 } expect(validateNumberField('50', config)).toEqual({ value: 50, errors: [], warnings: [], }) }) it('should warn about values below minimum', () => { const config: NumberValidationConfig = { min: 0, max: 100 } expect(validateNumberField('-1', config)).toEqual({ value: -1, errors: [], warnings: ['Must be greater than 0'], }) }) it('should warn about values above maximum', () => { const config: NumberValidationConfig = { min: 0, max: 100 } expect(validateNumberField('101', config)).toEqual({ value: 101, errors: [], warnings: ['Must be less than 100'], }) })This approach enhances test clarity and makes it easier to identify which specific constraint is failing if a test breaks.
60-75: LGTM with suggestion: Precision and scale validation is well-implemented.This test case effectively verifies the behavior of
validateNumberFieldwhen validating precision and scale constraints. It covers scenarios for numbers within and exceeding the specified constraints.Consider adding more specific test cases to cover boundary conditions explicitly. For example:
it('should validate precision and scale at exact boundaries', () => { const config: NumberValidationConfig = { precision: 5, scale: 2 } expect(validateNumberField('999.99', config)).toEqual({ value: 999.99, errors: [], warnings: [], }) }) it('should warn when precision is exceeded but scale is within limit', () => { const config: NumberValidationConfig = { precision: 5, scale: 2 } expect(validateNumberField('1000.00', config)).toEqual({ value: 1000.00, errors: [], warnings: ['Must have at most 3 digits before the decimal point'], }) }) it('should warn when scale is exceeded but precision is within limit', () => { const config: NumberValidationConfig = { precision: 5, scale: 2 } expect(validateNumberField('999.999', config)).toEqual({ value: 999.999, errors: [], warnings: ['Must have at most 2 digits after the decimal point'], }) })These additional tests would provide more comprehensive coverage of the precision and scale validation logic.
77-91: LGTM with suggestions: Currency format validation is implemented correctly.This test case verifies the basic behavior of
validateNumberFieldwhen validating currency format. It covers scenarios for both valid and invalid currency formats.Consider expanding the test cases to cover more currency format scenarios:
- Test with currency symbols:
$100.00,€100,00- Test with different thousand separators:
1,000.00,1.000,00- Test with negative values:
-100.00- Test with zero:
0.00,$0.00- Test with very large numbers:
1,000,000.00For example:
it('should validate currency format with symbols', () => { const config: NumberValidationConfig = { currency: true } expect(validateNumberField('$100.00', config)).toEqual({ value: 100, errors: [], warnings: [], }) }) it('should validate negative currency values', () => { const config: NumberValidationConfig = { currency: true } expect(validateNumberField('-100.00', config)).toEqual({ value: -100, errors: [], warnings: [], }) })These additional tests would provide more comprehensive coverage of various currency format scenarios.
93-105: LGTM with suggestions: Step constraint validation is implemented correctly.This test case verifies the basic behavior of
validateNumberFieldwhen validating the step constraint. It covers scenarios for both compliant and non-compliant values.Consider expanding the test cases to cover more step constraint scenarios:
- Test with zero as a valid step value:
0.0- Test with a negative value that complies with the step
- Test with different step sizes (e.g., 1, 0.1, 0.01)
- Test with edge cases (e.g., very small or very large steps)
For example:
it('should validate step constraint with zero', () => { const config: NumberValidationConfig = { step: 0.5 } expect(validateNumberField('0.0', config)).toEqual({ value: 0, errors: [], warnings: [], }) }) it('should validate step constraint with negative values', () => { const config: NumberValidationConfig = { step: 0.5 } expect(validateNumberField('-2.5', config)).toEqual({ value: -2.5, errors: [], warnings: [], }) }) it('should validate step constraint with very small step', () => { const config: NumberValidationConfig = { step: 0.001 } expect(validateNumberField('0.002', config)).toEqual({ value: 0.002, errors: [], warnings: [], }) })These additional tests would provide more comprehensive coverage of various step constraint scenarios.
107-117: LGTM with suggestions: Thousands separator and decimal point handling is implemented correctly.This test case verifies the basic behavior of
validateNumberFieldwhen handling thousands separators and decimal points. It covers a scenario with both separators present.Consider expanding the test cases to cover more number formatting scenarios:
- Test with only thousands separators:
1,234,567- Test with only decimal point:
1234.56- Test with different separator configurations (e.g., European format:
1.234,56)- Test with negative numbers:
-1,234.56- Test with edge cases (e.g., very large numbers, numbers with many decimal places)
For example:
it('should handle numbers with only thousands separators', () => { const config: NumberValidationConfig = { thousandsSeparator: ',', decimalPoint: '.', } expect(validateNumberField('1,234,567', config)).toEqual({ value: 1234567, errors: [], warnings: [], }) }) it('should handle European number format', () => { const config: NumberValidationConfig = { thousandsSeparator: '.', decimalPoint: ',', } expect(validateNumberField('1.234,56', config)).toEqual({ value: 1234.56, errors: [], warnings: [], }) }) it('should handle negative numbers with separators', () => { const config: NumberValidationConfig = { thousandsSeparator: ',', decimalPoint: '.', } expect(validateNumberField('-1,234.56', config)).toEqual({ value: -1234.56, errors: [], warnings: [], }) })These additional tests would provide more comprehensive coverage of various number formatting scenarios.
119-136: LGTM with suggestions: Special types validation is implemented correctly.This test case effectively verifies the behavior of
validateNumberFieldwhen validating special number types (prime and odd). It covers scenarios for numbers that meet both criteria, one criterion, or neither.Consider expanding the test cases to cover more special type scenarios and edge cases:
- Test with more special types if supported (e.g., even, perfect square, Fibonacci number)
- Test with edge cases (e.g., 0, 1, very large prime numbers)
- Test with negative numbers
- Test with decimal numbers
For example:
it('should validate edge cases for prime and odd', () => { const config: NumberValidationConfig = { specialTypes: ['prime', 'odd'] } expect(validateNumberField('2', config)).toEqual({ value: 2, errors: [], warnings: ['Must be an odd number'], }) expect(validateNumberField('1', config)).toEqual({ value: 1, errors: [], warnings: ['Must be a prime number'], }) }) it('should handle negative numbers for special types', () => { const config: NumberValidationConfig = { specialTypes: ['prime', 'odd'] } expect(validateNumberField('-3', config)).toEqual({ value: -3, errors: [], warnings: ['Must be a prime number'], }) }) it('should validate additional special types if supported', () => { const config: NumberValidationConfig = { specialTypes: ['even', 'perfectSquare'] } expect(validateNumberField('16', config)).toEqual({ value: 16, errors: [], warnings: [], }) expect(validateNumberField('18', config)).toEqual({ value: 18, errors: [], warnings: ['Must be a perfect square'], }) })These additional tests would provide more comprehensive coverage of various special type scenarios and edge cases.
138-145: LGTM with suggestions: Number rounding is implemented correctly.This test case verifies the basic behavior of
validateNumberFieldwhen the 'round' option is enabled. It covers a scenario where a number is rounded up.Consider expanding the test cases to cover more rounding scenarios:
- Test rounding down (e.g., 3.2 to 3)
- Test rounding at the midpoint (e.g., 3.5 to 4)
- Test rounding negative numbers
- Test rounding with different decimal places
For example:
it('should round numbers down when configured', () => { const config: NumberValidationConfig = { round: true } expect(validateNumberField('3.2', config)).toEqual({ value: 3, errors: [], warnings: [], }) }) it('should round numbers at midpoint when configured', () => { const config: NumberValidationConfig = { round: true } expect(validateNumberField('3.5', config)).toEqual({ value: 4, errors: [], warnings: [], }) }) it('should round negative numbers when configured', () => { const config: NumberValidationConfig = { round: true } expect(validateNumberField('-3.7', config)).toEqual({ value: -4, errors: [], warnings: [], }) }) it('should round to specified decimal places when configured', () => { const config: NumberValidationConfig = { round: true, scale: 2 } expect(validateNumberField('3.456', config)).toEqual({ value: 3.46, errors: [], warnings: [], }) })These additional tests would provide more comprehensive coverage of various rounding scenarios.
147-154: LGTM with suggestions: Number truncation is implemented correctly.This test case verifies the basic behavior of
validateNumberFieldwhen the 'truncate' option is enabled. It covers a scenario where a number is truncated.Consider expanding the test cases to cover more truncation scenarios:
- Test truncating a number with multiple decimal places
- Test truncating negative numbers
- Test truncating to a specified number of decimal places
- Test truncating numbers very close to the next integer
For example:
it('should truncate numbers with multiple decimal places', () => { const config: NumberValidationConfig = { truncate: true } expect(validateNumberField('3.999', config)).toEqual({ value: 3, errors: [], warnings: [], }) }) it('should truncate negative numbers', () => { const config: NumberValidationConfig = { truncate: true } expect(validateNumberField('-3.7', config)).toEqual({ value: -3, errors: [], warnings: [], }) }) it('should truncate to specified decimal places', () => { const config: NumberValidationConfig = { truncate: true, scale: 2 } expect(validateNumberField('3.456', config)).toEqual({ value: 3.45, errors: [], warnings: [], }) }) it('should truncate numbers very close to the next integer', () => { const config: NumberValidationConfig = { truncate: true } expect(validateNumberField('3.99999999', config)).toEqual({ value: 3, errors: [], warnings: [], }) })These additional tests would provide more comprehensive coverage of various truncation scenarios.
1-155: Overall, the test suite is well-implemented with room for enhancement.The test suite for
validateNumberFieldis comprehensive and covers all major functionalities, including basic validation, various constraints, special types, rounding, and truncation. The tests are clear, concise, and follow good testing practices.To further improve the test suite, consider the following suggestions:
Increase edge case coverage: Add more tests for boundary conditions, extreme values, and special cases for each functionality.
Improve test isolation: Split some of the larger test cases into smaller, more focused tests to improve readability and maintainability.
Add negative test cases: Include tests that verify the function's behavior with invalid configurations or unexpected inputs.
Test combinations of constraints: Add tests that verify the behavior when multiple constraints are applied simultaneously.
Consider using property-based testing: Implement property-based tests for functions where a wide range of inputs need to be tested, such as the basic number validation.
Add tests for error messages: Verify that the error and warning messages are correct and consistent across different scenarios.
Test performance: If performance is a concern, consider adding benchmarks or performance tests for large inputs or complex configurations.
By implementing these suggestions, you can create an even more robust and comprehensive test suite for the
validateNumberFieldfunction.
📜 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/number/package.jsonis excluded by!**/*.json
📒 Files selected for processing (6)
- validate/number/README.MD (1 hunks)
- validate/number/jest.config.js (1 hunks)
- validate/number/rollup.config.mjs (1 hunks)
- validate/number/src/index.ts (1 hunks)
- validate/number/src/validate.number.plugin.spec.ts (1 hunks)
- validate/number/src/validate.number.plugin.ts (1 hunks)
🚧 Files skipped from review as they are similar to previous changes (2)
- validate/number/rollup.config.mjs
- validate/number/src/index.ts
🧰 Additional context used
🪛 LanguageTool
validate/number/README.MD
[uncategorized] ~61-~61: Loose punctuation mark.
Context: ...ect with the following options: -min: Minimum allowed value -max: Maximum ...(UNLIKELY_OPENING_PUNCTUATION)
🪛 Biome
validate/number/src/validate.number.plugin.ts
[error] 42-42: isNaN is unsafe. It attempts a type coercion. Use Number.isNaN instead.
See the MDN documentation for more details.
Unsafe fix: Use Number.isNaN instead.(lint/suspicious/noGlobalIsNan)
🔇 Additional comments (16)
validate/number/jest.config.js (1)
1-16: LGTM! Well-structured Jest configuration.The Jest configuration is well-organized and follows best practices for a Node.js application with TypeScript support. Here are some observations and suggestions:
The use of shared setup files (
../../test/dotenv-config.js,../../test/betterConsoleLog.js, etc.) promotes consistency across test suites. However, be cautious of potential tight coupling between test suites.The
testTimeoutis set to 60 seconds, which is quite long for unit tests. Consider reviewing your test suite to identify any long-running tests that might benefit from optimization.The
forceExitoption is enabled, which can mask issues with asynchronous operations not being properly cleaned up. It's generally better to ensure all async operations are properly handled in your tests.The
passWithNoTestsoption is enabled, which can be useful in CI/CD pipelines but might hide the fact that no tests are running. Ensure this aligns with your testing strategy.To ensure consistency across your project, let's verify if similar Jest configurations exist in other directories:
This will help identify any inconsistencies in Jest configurations across your project.
✅ Verification successful
The Jest configuration in
validate/number/jest.config.jsis consistent with other configurations in the codebase. ThetestTimeoutvalue of 60,000 ms aligns with similar settings elsewhere, and the use of shared setup files maintains consistency without introducing tight coupling.🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash # Description: Check for other Jest config files and compare their content # Find all Jest config files config_files=$(fd jest.config.js) # Compare the content of each file with this one for file in $config_files do if [ "$file" != "validate/number/jest.config.js" ]; then echo "Comparing with $file:" diff -u validate/number/jest.config.js "$file" echo fi doneLength of output: 14269
validate/number/README.MD (6)
1-10: LGTM: Header and infocard section are well-structured.The header and infocard section provide clear and concise information about the plugin and its event type. The formatting is correct, and the content is informative.
11-22: LGTM: Features section is comprehensive and well-formatted.The Features section provides a clear and detailed list of the plugin's capabilities. The use of bullet points makes it easy to read and understand the range of functionalities offered by the plugin.
23-30: LGTM: Installation section is correct and the package name issue has been resolved.The Installation section provides clear instructions for installing the plugin. The package name in the installation command now correctly matches the one used in the import statement, addressing the issue raised in a previous review.
31-55: LGTM: Example Usage section is comprehensive and well-formatted.The Example Usage section provides a clear and detailed code snippet that demonstrates how to use the plugin with various configuration options. The code is well-formatted and serves as a good reference for users implementing the plugin.
75-81: LGTM: Behavior section provides clear operational details.The Behavior section effectively describes how the plugin operates, including its event listening, validation process, and error handling. The mention of the default field name ('numberField') is consistent with the need for a
fieldNameoption in the configuration section.
57-74:⚠️ Potential issueAdd the missing
fieldNameconfiguration option.The Configuration section provides a comprehensive list of options, and the formatting issue with extra spaces has been resolved. However, the
fieldNameoption is still missing from the list. This option is important as it allows users to specify which field to validate.Add the following line to the configuration options list:
The `validateNumber` accepts a configuration object with the following options: +- `fieldName`: The name of the field to validate (default is 'numberField') - `min`: Minimum allowed value - `max`: Maximum allowed valueThis addition will ensure that users are aware of all available configuration options.
🧰 Tools
🪛 LanguageTool
[uncategorized] ~61-~61: Loose punctuation mark.
Context: ...ect with the following options: -min: Minimum allowed value -max: Maximum ...(UNLIKELY_OPENING_PUNCTUATION)
validate/number/src/validate.number.plugin.spec.ts (5)
1-4: LGTM: Imports are correctly defined.The necessary functions and types are imported from the correct module.
6-8: LGTM: Test suite structure is well-defined.The test suite is correctly set up using Jest's
describefunction, and a default configuration object is appropriately defined.
9-16: LGTM: Basic number validation test is well-implemented.This test case effectively checks the basic functionality of
validateNumberFieldfor a valid number input.
18-25: LGTM: Non-numeric input handling is correctly tested.This test case properly verifies the behavior of
validateNumberFieldwhen given invalid, non-numeric input.
46-58: LGTM: Integer constraint validation is properly tested.This test case effectively verifies the behavior of
validateNumberFieldwhen theintegerOnlyconstraint is applied. It correctly tests both integer and non-integer inputs.validate/number/src/validate.number.plugin.ts (4)
1-24: Well-structured interfaces and appropriate imports.The imports and interface definitions are clear, comprehensive, and appropriate for the number validation plugin. The
NumberValidationConfiginterface provides a wide range of options for customizing the validation process, while theNumberValidationResultinterface effectively structures the output.
47-57: Correct handling of rounding, truncation, and integer validation.The implementation of rounding, truncation, and integer-only validation is correct and follows the configuration options. The use of
Number.isIntegerfor checking integer values is appropriate.
131-133:⚠️ Potential issueRemove the redundant
return recordstatement inside the loop.The
return recordstatement within the loop causes the function to exit after processing the first field, skipping validation for the remaining fields. Removing this statement allows all specified fields to be validated.Apply this diff to fix the issue:
if (typeof value !== 'string' && typeof value !== 'number') { - return record + continue }Likely invalid or redundant comment.
59-77:⚠️ Potential issueCorrect the comparison logic for minimum and maximum value validation.
The logic for both minimum and maximum value validation with inclusivity needs adjustment. When
inclusiveistrue, the conditions should allow values equal toconfig.minorconfig.maxrespectively.Apply these diffs to fix the logic:
For minimum value validation:
- config.inclusive ? numberValue < config.min : numberValue <= config.min + config.inclusive ? numberValue < config.min : numberValue <= config.minFor maximum value validation:
- config.inclusive ? numberValue > config.max : numberValue >= config.max + config.inclusive ? numberValue > config.max : numberValue >= config.maxLikely invalid or redundant comment.
carlbrugger
left a comment
carlbrugger
left a comment
There was a problem hiding this comment.
Other than version number, this looks good
2 participants
Please explain how to summarize this PR for the Changelog:
Tell code reviewer how and what to test: