Fea: new date formates 9652 #9686
Conversation
|
|
📝 WalkthroughWalkthroughThe changes involve updates to documentation and code to support additional date formats in the Changes
Sequence Diagram(s)sequenceDiagram
participant User
participant DateField
participant DateTimeHelper
participant ConvertDateFormat
User->>DateField: Input date in new format
DateField->>DateTimeHelper: Parse date
DateTimeHelper-->>DateField: Return parsed date
DateField->>ConvertDateFormat: Convert date format
ConvertDateFormat-->>DateField: Return converted date
DateField-->>User: Display date
Thank you for using CodeRabbit. We offer it for free to the OSS community and would appreciate your support in helping us grow. If you find it useful, would you consider giving us a shout-out on your favorite social media? 🪧 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 (
|
There was a problem hiding this comment.
Actionable comments posted: 1
🧹 Outside diff range and nitpick comments (7)
packages/noco-docs/docs/070.fields/040.field-types/070.date-time-based/020.date.md (2)
33-34: LGTM! New date formats added correctly.The new date formats "DD MMM YYYY" and "DD MMM YY" have been added correctly to the "Supported date formats" table. These additions align with the PR objectives and the AI-generated summary, enhancing the flexibility of date input options as intended.
Consider adding a brief explanation of what "MMM" represents (abbreviated month names) in the table or in a note below the table, to ensure clarity for all users.
Line range hint
1-34: Consider enhancing the documentation for better clarity and organization.The additions to the supported date formats table are valuable. To further improve the document, consider the following suggestions:
Reorder the table entries to group similar formats together. For example, place the new "DD MMM YYYY" and "DD MMM YY" formats next to other day-first formats.
Add a brief introduction to the new formats in the document, explaining their significance and use cases.
Include a note about the flexibility these new formats provide, especially for international date representations.
Here's a suggested reordering of the table entries:
| Format | Example | |--------------|--------------| | YYYY-MM-DD | 2023-09-22 | | YYYY/MM/DD | 2023/09/22 | | YYYY MM DD | 2023 09 22 | | DD-MM-YYYY | 22-09-2023 | | DD/MM/YYYY | 22/09/2023 | | DD MM YYYY | 22 09 2023 | | DD MMM YYYY | 22 JAN 2024 | | DD MMM YY | 22 JAN 24 | | MM-DD-YYYY | 09-22-2023 | | MM/DD/YYYY | 09/22/2023 | | MM DD YYYY | 09 22 2023 |Consider adding a brief explanation before the table:
"NocoDB now supports a wide range of date formats, including formats with abbreviated month names (MMM). This enhancement provides greater flexibility for international date representations."
packages/noco-docs/docs/070.fields/040.field-types/070.date-time-based/010.date-time.md (2)
34-35: LGTM! New date formats added correctly.The new date formats "DD MMM YYYY" and "DD MMM YY" have been added to the "Supported date formats" table. These additions are consistent with the existing format and provide clear examples.
Some observations:
- The new formats are correctly aligned with the table structure.
- The examples provided ("22 JAN 2024" and "22 JAN 24") are clear and follow the specified format.
- These additions enhance the flexibility of date representations as mentioned in the PR objectives.
Consider adding a note or link to documentation about localization, as the month abbreviations (e.g., "JAN") might vary in different languages.
Image
datetime.pngis Missing from the Expected LocationThe referenced image
/img/v2/fields/types/datetime.pngdoes not exist in the repository. Please ensure that the image is added topackages/noco-docs/public/img/v2/fields/types/or update the documentation to reference the correct image path.🔗 Analysis chain
Line range hint
1-35: Consider updating other sections for consistency.The new date formats have been successfully added to the "Supported date formats" table. To ensure complete documentation:
- Review the "Create a date time field" section (steps 4-5) to check if it needs updating to mention the new format options.
- Consider adding an example using one of the new formats in the main description or in the field creation steps.
- Verify if the screenshot (
/img/v2/fields/types/datetime.png) needs updating to reflect these new format options.To check if the image needs updating, please run:
This will help determine if the image needs to be updated to reflect the new format options.
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash # Description: Check if the datetime field image exists and when it was last modified # Test: Check if the image exists and its last modification date if [ -f "packages/noco-docs/public/img/v2/fields/types/datetime.png" ]; then echo "Image exists. Last modified:" stat -c %y "packages/noco-docs/public/img/v2/fields/types/datetime.png" else echo "Image does not exist at the expected location." fiLength of output: 174
packages/nocodb/src/helpers/convertDateFormat.ts (2)
20-23: LGTM! Consider a minor optimization.The new date format conditions for 'DD MMM YYYY' and 'DD MMM YY' are correctly implemented and consistent with the existing code structure. The use of '%b' for month abbreviation and the differentiation between '%Y' and '%y' for year formats are appropriate.
Consider combining the database type check to reduce repetition:
- } else if (date_format === 'DD MMM YYYY') { - if (type === 'mysql2' || type === 'sqlite3') return '%d %b %Y'; - } else if (date_format === 'DD MMM YY') { - if (type === 'mysql2' || type === 'sqlite3') return '%d %b %y'; + } else if (type === 'mysql2' || type === 'sqlite3') { + if (date_format === 'DD MMM YYYY') return '%d %b %Y'; + if (date_format === 'DD MMM YY') return '%d %b %y'; }This change would slightly improve readability and reduce the nesting level.
Line range hint
1-27: Consider future refactoring for improved maintainability.The new date format additions are well-integrated and don't impact existing functionality. The function remains consistent in its approach to format conversion.
For future improvements, consider refactoring this function to use a mapping object or switch statement. This could enhance readability and make it easier to add or modify formats in the future. For example:
const formatMap = { 'YYYY-MM-DD': '%Y-%m-%d', 'YYYY/MM/DD': '%Y/%m/%d', // ... other formats ... 'DD MMM YYYY': '%d %b %Y', 'DD MMM YY': '%d %b %y', }; export function convertDateFormat(date_format: string, type: string) { if (type === 'mysql2' || type === 'sqlite3') { return formatMap[date_format] || date_format; } return date_format; }This approach would make the function more scalable and easier to maintain as new date formats are added.
packages/nocodb-sdk/src/lib/dateTimeHelper.ts (1)
30-32: Approve new date formats with minor suggestions.The addition of 'DD MMM YYYY' and 'DD MMM YY' formats is good and aligns with the PR objective. However, there are a few minor points to address:
The comment could be improved for clarity and consistency with the codebase style. Consider changing it to:
// Added 2 new formats as per issue #9652Use consistent quote style for string literals. Preferably single quotes to match the existing entries:
'DD MMM YYYY', 'DD MMM YY'
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
📒 Files selected for processing (4)
- packages/noco-docs/docs/070.fields/040.field-types/070.date-time-based/010.date-time.md (1 hunks)
- packages/noco-docs/docs/070.fields/040.field-types/070.date-time-based/020.date.md (1 hunks)
- packages/nocodb-sdk/src/lib/dateTimeHelper.ts (1 hunks)
- packages/nocodb/src/helpers/convertDateFormat.ts (1 hunks)
🧰 Additional context used
🔇 Additional comments (2)
packages/noco-docs/docs/070.fields/040.field-types/070.date-time-based/010.date-time.md (1)
Line range hint
1-35: Approved: New date formats successfully added.This review has covered the following aspects:
- The new date formats "DD MMM YYYY" and "DD MMM YY" have been correctly added to the "Supported date formats" table.
- The additions are consistent with the existing document structure and enhance the flexibility of date representations.
- The overall document remains well-structured and informative.
Suggestions for further improvement have been provided in previous comments, including:
- Considering a note about localization for month abbreviations.
- Reviewing other sections of the document for consistency with the new formats.
- Verifying if the associated image needs updating.
These suggestions are minor and do not impact the approval of the current changes. Great job on expanding the supported date formats!
packages/nocodb-sdk/src/lib/dateTimeHelper.ts (1)
Line range hint
1-32: Summary of changes and recommendationsThe changes to
dateTimeHelper.tsare minimal and focused, successfully adding two new date formats as requested. The implementation is correct and doesn't require changes to existing functions, which is positive for maintaining current functionality.To further improve this PR:
- Address the minor code style suggestions for consistency.
- Consider adding tests to verify the new formats work correctly with existing functions.
- If applicable, update any relevant documentation to reflect the newly supported date formats.
Overall, good job on implementing this feature with minimal impact on the existing codebase.
| ///added 2 new format#9652 | ||
| 'DD MMM YYYY', | ||
| "DD MMM YY" |
There was a problem hiding this comment.
💡 Codebase verification
Add tests for the newly introduced date formats.
No existing tests cover the new date formats 'DD MMM YYYY' and "DD MMM YY". To ensure these formats are correctly validated and parsed across all relevant functions, please add the following tests:
-
Validation Tests:
- Verify that
validateDateWithUnknownFormatcorrectly validates dates in the new formats.
- Verify that
-
Format Identification Tests:
- Ensure that
getDateFormataccurately identifies and returns the new formats.
- Ensure that
-
Date-Time Parsing Tests:
- Confirm that
getDateTimeFormathandles the new date formats in combination with existing time formats.
- Confirm that
Implementing these tests will help maintain the robustness of the codebase and prevent potential issues related to date handling.
🔗 Analysis chain
Consider adding tests for new date formats.
The addition of new date formats is straightforward and doesn't require changes to existing functions. However, to ensure robustness, consider adding tests that verify:
validateDateWithUnknownFormatcorrectly validates dates in the new formats.getDateFormatcorrectly identifies and returns the new formats.getDateTimeFormatworks correctly with the new date formats combined with existing time formats.
This will help catch any potential issues and ensure the new formats integrate smoothly with the existing codebase.
To assist in verifying the impact and usage of these new formats, you can run the following script:
This script will help identify if there are existing tests for the new formats and where date format validation or parsing is used in the codebase, which might need to be updated or tested with the new formats.
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash
# Description: Check for usage and tests of new date formats
# Test: Search for usage of new date formats in test files
echo "Searching for tests of new date formats:"
rg --type typescript -g '*test*' -i "DD MMM (YYYY|YY)"
# Test: Search for usage of date format validation or parsing in the codebase
echo "Searching for date format validation or parsing usage:"
rg --type typescript 'validateDateWithUnknownFormat|getDateFormat|getDateTimeFormat'
Length of output: 423
Script:
#!/bin/bash
# Description: Check for usage and tests of new date formats with corrected file type patterns
# Test: Search for usage of new date formats in test files
echo "Searching for tests of new date formats:"
rg -g "*.ts" -g "*.tsx" -i "DD MMM (YYYY|YY)"
# Test: Search for usage of date format validation or parsing in the codebase
echo "Searching for date format validation or parsing usage:"
rg -g "*.ts" -g "*.tsx" 'validateDateWithUnknownFormat|getDateFormat|getDateTimeFormat'
Length of output: 3601
Change Summary
Added new date formates requested in - #9652
Change type
Test/ Verification
Provide summary of changes.
Additional information / screenshots (optional)
Anything for maintainers to be made aware of
If im missing something please mention it i will do it