Skip to content

Conversation

Copilot
Copy link
Contributor

@Copilot Copilot AI commented Sep 22, 2025

  • Explore repository structure and understand current implementation
  • Analyze existing TimeUnit.ParseTime method
  • Verify DateMath already supports y, M, w units
  • Examine TimeSpanExtensions with year/month/week constants
  • Build and test current codebase (550 tests passing)
  • Add support for parsing "y" (years) using AvgDaysInAYear constant
  • Add support for parsing "M" (months) using AvgDaysInAMonth constant
  • Add support for parsing "w" (weeks) using 7 days
  • Add comprehensive test cases for new time units
  • Ensure case-sensitive handling (M vs m)
  • Test edge cases and integration with existing functionality
  • Verify all 572 tests pass with new functionality
  • Manual testing confirms correct parsing and conversion
  • Improve ParseTime method based on code review feedback
    • Add early null/empty string validation with trim
    • Use normalized values consistently for all EndsWith checks
    • Add comprehensive whitespace handling tests
    • Add special character validation tests
    • All 594 tests pass (added 22 new test cases)
  • Refactor test methods to follow testing best practices
    • Apply MethodName_StateUnderTest_ExpectedBehavior naming convention
    • Improve AAA (Arrange, Act, Assert) pattern clarity
    • Break down complex conditional test logic into focused, single-purpose tests
    • All 603 tests pass (9 additional refined test methods)

Summary

Successfully implemented support for Elasticsearch-compatible time units (y, M, w) in the TimeUnit parser with improved robustness and comprehensive test coverage following established best practices.

Latest Improvements (addressing @niemyjski testing best practices feedback):

  • Proper naming convention: Renamed all test methods to follow MethodName_StateUnderTest_ExpectedBehavior pattern
  • Improved test structure: Enhanced AAA pattern clarity with clear Arrange, Act, Assert sections
  • Focused test methods: Replaced complex conditional logic with dedicated, single-purpose tests:
    • Parse_UppercaseM_ParsesAsMonths() - Tests uppercase M for months
    • Parse_LowercaseM_ParsesAsMinutes() - Tests lowercase m for minutes
    • Parse_MonthsAndMinutes_ProduceDifferentResults() - Verifies case sensitivity
    • Parse_YearUnit_ReturnsExpectedDays() - Dedicated year unit testing
    • Parse_MonthUnit_ReturnsExpectedDays() - Dedicated month unit testing
    • Parse_WeekUnit_ReturnsExpectedDays() - Dedicated week unit testing

Previously Completed Improvements:

  • Early validation: Added null/empty checks and trimming at method start
  • Consistent parsing: All EndsWith checks now use normalized (trimmed) values
  • Enhanced test coverage: Added comprehensive whitespace handling and special character validation tests

Key Features Added:

  • Years (y): Converts to TimeSpan using TimeSpanExtensions.AvgDaysInAYear (365.2425 days)
  • Months (M): Converts to TimeSpan using TimeSpanExtensions.AvgDaysInAMonth (30.436875 days)
  • Weeks (w): Converts to TimeSpan using exactly 7 days
  • Case-sensitive parsing: "M" = months, "m" = minutes (existing behavior preserved)

Implementation Details:

  • Extended TimeUnit.ParseTime method with improved validation
  • Early return for null/empty strings after trimming
  • Consistent use of normalized values for all parsing logic
  • Maintains support for negative values and whitespace inputs
  • Used existing constants from TimeSpanExtensions for consistency

Testing:

  • All 603 tests pass (original 550 + 53 new/refined TimeUnit tests)
  • Follows Microsoft's unit testing best practices and repository guidelines
  • Comprehensive coverage including whitespace handling and special character rejection
  • Proper test method naming and structure following MethodName_StateUnderTest_ExpectedBehavior convention
  • Clear AAA pattern implementation
  • Manual testing confirms correct parsing with various input formats
  • Maintains backward compatibility with existing functionality

The implementation aligns with Elasticsearch time unit conventions and integrates seamlessly with the existing DateMath functionality.

Fixes #2.


✨ Let Copilot coding agent set things up for you — coding agent works faster and does higher quality work when set up for your repo.

@CLAassistant
Copy link

CLAassistant commented Sep 22, 2025

CLA assistant check
Thank you for your submission! We really appreciate it. Like many open source projects, we ask that you all sign our Contributor License Agreement before we can accept your contribution.
0 out of 2 committers have signed the CLA.

❌ Copilot
❌ niemyjski
You have signed the CLA already but the status is still pending? Let us recheck it.

@Copilot Copilot AI changed the title [WIP] Update timeunit to support years, weeks and months Add support for years (y), months (M), and weeks (w) time units to TimeUnit parser Sep 22, 2025
@Copilot Copilot AI requested a review from niemyjski September 22, 2025 04:28
Copilot finished work on behalf of niemyjski September 22, 2025 04:28
… values consistently, and add comprehensive whitespace/special character tests

Co-authored-by: niemyjski <[email protected]>
@Copilot Copilot AI requested a review from niemyjski September 22, 2025 13:52
Copilot finished work on behalf of niemyjski September 22, 2025 13:52
…onvention, improve AAA pattern, and simplify test logic

Co-authored-by: niemyjski <[email protected]>
Copilot finished work on behalf of niemyjski September 22, 2025 13:58
Apply suggestion from @niemyjski
Apply suggestion from @niemyjski
Apply suggestion from @niemyjski
Apply suggestion from @niemyjski

ran dotnet format
@niemyjski niemyjski marked this pull request as ready for review September 22, 2025 14:23
@niemyjski niemyjski merged commit c05fbf4 into main Sep 22, 2025
2 of 3 checks passed
@niemyjski niemyjski deleted the copilot/fix-2 branch September 22, 2025 14:23
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Development

Successfully merging this pull request may close these issues.

Update timeunit to support years, weeks and months

3 participants